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

Rename [RegexGenerator] to [GeneratedRegex] #72434

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Jul 19, 2022

Renamed [RegexGenerator] to [GeneratedRegex], updated its usages, renamed analyzer & code fixer, and updated the docs. The internal implementation and some tests use the term RegexGenerator in a few places, which were making less sense when I tried renaming to either GeneratedRegex or GeneratedRegexAttribute, so I have left those files alone.

Closes #62123,

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2022
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jul 19, 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

Renamed [RegexGenerator] to [Regex], updated its usages, renamed analyzer & code fixer, and updated the docs. The internal implementation and some tests use the term RegexGenerator in a few places, which were making less sense when I tried renaing to either Regex or RegexAttribute, so I have left those 14 files alone.

Closes #62123,

Author: am11
Assignees: -
Labels:

area-System.Text.RegularExpressions, new-api-needs-documentation, community-contribution

Milestone: -

@am11
Copy link
Member Author

am11 commented Jul 19, 2022

@stephentoub, @joperezr, does it need API review? I have opened it as a Draft in case review is needed.

@teo-tsirpanis
Copy link
Contributor

It almost certainly needs an API review.

@joperezr
Copy link
Member

Thanks for opening the PR @am11. Yes, it does require API review, we marked the corresponding issue as blocking to see if we could get to it in today's API Review but we ran out of time. We will likely go over it on Thursday at 10am, if you want to join, they are usually broadcasted live here: https://www.youtube.com/channel/UCiaZbznpWV1o-KLxj8zqR6A

@joperezr
Copy link
Member

This PR will also have to update the analyzer and codefixer for the regex source generator:

private const string RegexGeneratorTypeName = "System.Text.RegularExpressions.RegexGeneratorAttribute";

@am11
Copy link
Member Author

am11 commented Jul 20, 2022

Thanks @joperezr.

This PR will also have to update the analyzer and codefixer

Yup. I updated both in the first commit.

@joperezr
Copy link
Member

We discussed this today in APIReview, the decision was to use [GeneratedRegex] instead of [Regex]. @am11 Would you be willing to rename the changes here to match that?

@stephentoub
Copy link
Member

Reiterating my comments from API review, runtime libraries flow at a different rate to dependent repos (e.g. dotnet/winforms) than does the generator. I suspect when this merges, we'll end up breaking one or more dependent repos when they try to pick up flowed runtime libraries. We'll end up having a few options I suspect:

  • Block updates of the libs until they also pick up a new SDK
  • Add GeneratedRegex as a new attribute, update the generator to support both attributes, and then later remove RegexGeneratorAttribute and the support for it once the updated SDK has flown everywhere needed.
  • Temporarily undo use of RegexGenerator in dependent repos until they can successfully use GeneratedRegex

I could be wrong, of course. Maybe everything will "just work" 😆

@joperezr
Copy link
Member

I agree that this could be problematic. After a quick search in source.dot.net seems like out of the repos where this could cause a problem, only winforms is actively using the attribute. This may or may not be very accurate since source.dot.net isn't a full representation of our repos consumption.

All that said, even though you are right saying that sdk and runtime libraries flow at different rates to these other repos, I'd imagine that the attribute and generator would both be part of the targeting pack, so in theory this shouldn't be a problem if someone is updating their targeting pack that they build against faster than their sdk. In any case, if/when the time comes of a break, I will evaluate the solutions proposed above, and figure out what the best/easiest workaround is. I do very much appreciate the heads up as it is a very valid concern 😄

@am11 am11 force-pushed the feature/generators/rename-regexgenerator branch from 478cdef to a65b50f Compare July 22, 2022 01:06
@am11 am11 changed the title Rename [RegexGenerator] to [Regex] Rename [RegexGenerator] to [GeneratedRegex] Jul 22, 2022
@am11 am11 marked this pull request as ready for review July 22, 2022 01:06
@am11 am11 requested a review from jeffhandley as a code owner July 22, 2022 01:06
Copy link
Member

@stephentoub stephentoub 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 working on this.

@joperezr
Copy link
Member

I'm seeing that we still have several places where we make reference to "RegexGenerator" in your code, some of them should definitely be changed like:

// Find all MethodDeclarationSyntax nodes attributed with RegexGenerator and gather the required information.

Some I'm not so sure if we should change, like the actual RegexGenerator generator type or the REGEXGENERATOR constant we use for when building production source. @stephentoub thoughts?

@stephentoub
Copy link
Member

Some I'm not so sure if we should change, like the actual RegexGenerator generator type or the REGEXGENERATOR constant we use for when building production source. @stephentoub thoughts?

We shouldn't change those. Those are about the generator.

@am11
Copy link
Member Author

am11 commented Jul 22, 2022

I was looking at the naming convention used by interop generator:

public sealed class LibraryImportGenerator : IIncrementalGenerator
and didn't wanted to use GeneratedRegexGenerator as it is redundant, so I left it as is (RegexGenerator).

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.

2 very minor comments but looks good otherwise. @am11 would you be able to fix the merge conflicts on the PR? If you need help with this let me know and I'm happy to do so.

@am11 am11 force-pushed the feature/generators/rename-regexgenerator branch from ac23089 to 7313080 Compare July 27, 2022 02:54
@am11
Copy link
Member Author

am11 commented Jul 27, 2022

I have rebased and resolved the merged conflicts. Failures are unrelated network/infra issues.

@joperezr joperezr merged commit b0090d0 into dotnet:main Jul 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconcile naming: RegexGeneratorAttribute to RegexAttribute
4 participants