Skip to content

Simplify UpgradeToRegexGenerator#72538

Merged
joperezr merged 5 commits intodotnet:mainfrom
Youssef1313:simplify-regex-gen
Jul 26, 2022
Merged

Simplify UpgradeToRegexGenerator#72538
joperezr merged 5 commits intodotnet:mainfrom
Youssef1313:simplify-regex-gen

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Jul 20, 2022 •
edited
Loading

Previously, analyzer reported argument indices in its properties. Then codefix will get the argument values from the indices in GetNode. I changed the analyzer to report the values directly, which simplifies things a little bit.

@ghost ghost added area-System.Text.RegularExpressions community-contribution Indicates that the PR has been added by a community member labels Jul 20, 2022
@ghost
Copy link

ghost commented Jul 20, 2022

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: Youssef1313
Assignees: -
Labels:

area-System.Text.RegularExpressions
Milestone: -

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 20, 2022

Marking as a draft. Not ready for review until #72534 is merged and this branch is rebased.

@Youssef1313 Youssef1313 marked this pull request as draft July 20, 2022 18:44
teo-tsirpanis
teo-tsirpanis reviewed Jul 20, 2022
View reviewed changes
src/libraries/System.Text.RegularExpressions/gen/UpgradeToRegexGeneratorCodeFixer.cs Outdated
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be renamed to something like FormatRegexOptions. It's not clear what it does by the name and parameter type.

Copy link
Member Author

@Youssef1313 Youssef1313 Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably stringifiedRegexOptions to make it clear this is a RegexOptions that we called ToString on?

@Youssef1313 Youssef1313 force-pushed the simplify-regex-gen branch 2 times, most recently from 6549876 to 8dab54b Compare July 21, 2022 04:31
@Youssef1313 Youssef1313 marked this pull request as ready for review July 21, 2022 04:33
@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 21, 2022

@joperezr Can you take a look please? Thanks!

@joperezr
Copy link
Member

joperezr commented Jul 21, 2022

Thanks for the PR @Youssef1313. Since you are changing this, one of the comments from the original PR that added the analyzer was particularly suggesting that ideally we shouldn't have the analyzer send any info into the fixer if possible in order to have both be easier to understand and maintain. Here is the relevant part of the comment by @mavasani:

Is there a reason to add this property bag to the diagnostic instead of computing all the required information in the fixer itself? Given the diagnostic location, the fixer can fetch the required syntax node from the location using root.FindNode(span) API. You can get the semantic model from the document using document.GetSemanticModelAsync API and then use semanticModel.GetOperation(node) API to get to the IInvocationOperation.
...
We generally don't recommend using such a property bag unless needed as you are now adding an additional dependency between the analyzer and fixer internals, which might make it more harder to maintain or understand.

I think if we are changing this now, we should re-evaluate that comment and perhaps moving the logic of grabbing the values out to the fixer itself. Thoughts?

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 21, 2022

Sure. I'll try with that approach.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 24, 2022

@joperezr I applied the suggestion. The PR is ready for another look.

@runfoapp runfoapp bot mentioned this pull request Jul 26, 2022
Closed
joperezr
joperezr reviewed Jul 26, 2022
View reviewed changes
src/libraries/System.Text.RegularExpressions/gen/UpgradeToRegexGeneratorAnalyzer.cs Outdated

// Validate that arguments pattern and options are constant and timeout was not passed in.
if (!TryValidateParametersAndExtractArgumentIndices(invocationOperation.Arguments, ref patternArgumentIndex, ref optionsArgumentIndex))
if (!TryValidateParametersAndExtractArgumentIndices(invocationOperation.Arguments))
Copy link
Member

@joperezr joperezr Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small NIT: The name of the method is technically not accurate any longer, now we are only doing validation of the Parameters so we could rename to ValidateParameters

joperezr
joperezr reviewed Jul 26, 2022
View reviewed changes
src/libraries/System.Text.RegularExpressions/gen/UpgradeToRegexGeneratorCodeFixer.cs Outdated
/// source generation.
/// </summary>
[ExportCodeFixProvider(LanguageNames.CSharp)]
[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
Copy link
Member

@joperezr joperezr Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like in other similar places we aren't adding this attribute, like:

runtime/src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/Analyzers/ConvertToLibraryImportFixer.cs

Line 23 in 39d82c9

[ExportCodeFixProvider(LanguageNames.CSharp)]

I'm not saying adding the attribute is wrong, but it also isn't very related to this PR so we should probably live that to a follow up PR.

joperezr
joperezr approved these changes Jul 26, 2022
View reviewed changes
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @Youssef1313 !

@joperezr
Copy link
Member

joperezr commented Jul 26, 2022

Will merge as soon as Build is green.

@joperezr joperezr merged commit 891cc3a into dotnet:main Jul 26, 2022
@Youssef1313 Youssef1313 deleted the simplify-regex-gen branch July 27, 2022 00:34
@runfoapp runfoapp bot mentioned this pull request Jul 27, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@joperezr joperezr joperezr approved these changes

+1 more reviewer

@teo-tsirpanis teo-tsirpanis teo-tsirpanis left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.RegularExpressions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Youssef1313 @joperezr @teo-tsirpanis