-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add RegexGenerator to System.ComponentModel.TypeConverter. #62325
Merged
stephentoub
merged 8 commits into
dotnet:main
from
Clockwork-Muse:regex_gen_62105_first
Feb 24, 2022
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2c41703
Add RegGen to System.ComponentModel.TypeConverter.
Clockwork-Muse 75b9dda
Change to targets generator enablement
Clockwork-Muse 177180d
Correct RegGen for System.ComponentModel.TypeConverter
Clockwork-Muse 5b96443
Remove extra space
Clockwork-Muse 95cadd0
Remove .NetF inclusion definition
Clockwork-Muse 154b7fd
Remove implicit regex generator entrypoint
Clockwork-Muse 8863e8f
Remove regex generator enabled variable target.
Clockwork-Muse e4af436
Update eng/generators.targets
joperezr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkoritzinsky what is the reasoning behind having an explicit path and an implicit path for adding the DLLImport generator? Is that reason also applicable to the RegexGenerator? Basically my question is: for the regexgenerator should we just simplify and only enable it whenever EnableRegexGenerator property is set to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needed the
ItemGroup
-based path to allow us to filter adding the generator ProjectReference based on referenced projects. With MSBuild static graph (which we use for our restore functionality), we can't add aProjectReference
in a target, so we're limited to evaluation time. Since properties are evaluated before items, we needed to do the generator reference add based on an item. The property was added for simplicity.Once we aren't adding sources to builds (and all of the APIs the generator uses are approved) then we can significantly simplify this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkoritzinsky it sounds like you wanted to make it automatic always and you see the property as less desirable, where @joperezr was saying the property is simpler. I do think a property is more WYSIWYG, and it seems acceptable for construction of the shared framework where we are more careful about layering/dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we wanted to make DllImportGenerator enabled automatically and the property was a fallback. We wanted to enable our generator automatically since we're consuming it throughout the runtime, including CoreLib. @stephentoub expressed a desire for us to enable it for all projects (including tests) and not even having a property for opt-in/out, but until we get all of our APIs approved, that's a little difficult.