Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: represent of CompilerItems and CompilerProperties as dedicated types #245

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

Corniel
Copy link
Contributor

@Corniel Corniel commented Jan 28, 2024

While debugging #244, I noticed that debugging which compiler properties where active was not really clear. So similar to #243, I introduced two dedicated (read-only) types that contain the properties and the items. Potentiality, it can replace the dictionaries in the AnalyzerResult. That is, obviously a breaking change.

Under the hood, CompilerProperties and CompilerItemsCollection are dictionaries (with case-insensitive keys), but as I think (that can be challenged) having the values without the related key have less value in this case, the TryGet() method returns a KeyValuePair like struct that combines the two.

Let me know what you think.

@daveaglick
Copy link
Collaborator

As with #243, I love where your thinking with these changes but just don't have the bandwidth to bring them in. #247 is a request for a new maintainer(s) and I think it makes sense to let whoever takes over the project decide the direction of open PRs. Thanks for the work, and hopefully we can find a maintainer soon and get this merged.

@Corniel Corniel marked this pull request as ready for review March 5, 2024 08:57
@Corniel
Copy link
Contributor Author

Corniel commented Mar 5, 2024

@phmonte I think this one is also ready for review. This one should be easy (I think(.

@phmonte phmonte merged commit 0f4b708 into phmonte:main Mar 7, 2024
6 checks passed
@Corniel Corniel deleted the compiler-properties branch March 8, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants