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

Reconcile naming: RegexGeneratorAttribute to RegexAttribute #62123

Closed
am11 opened this issue Nov 29, 2021 · 12 comments · Fixed by #72434
Closed

Reconcile naming: RegexGeneratorAttribute to RegexAttribute #62123

am11 opened this issue Nov 29, 2021 · 12 comments · Fixed by #72434
Labels
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@am11
Copy link
Member

am11 commented Nov 29, 2021

Re: #62105 (comment).

In the upcoming .NET 7, we have [GeneratedDllImport] and [RegexGenerator] attributes to annotate partial methods for source generators.

It would be nice to match their names at this early stage, preferably by renaming [GeneratedDllImport] to [DllImportGenerator].

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 29, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Nov 29, 2021

We also have prior art shipped in .NET 6:

I like that the .NET 6 names that they do include Generator or Generated at all. It makes them more succinct. Should we stick to this pattern for the new source generators as much as possible?

@jkotas
Copy link
Member

jkotas commented Nov 29, 2021

For interop source generator, we can use this as an opportunity to switch to more platform neutral naming. Dll is Windows-specific. How about calling the attribute [LibraryImport]?

@ghost
Copy link

ghost commented Nov 29, 2021

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

Issue Details

Re: #62105 (comment).

In the upcoming .NET 7, we have [GeneratedDllImport] and [RegexGenerator] attributes to annotate partial methods for source generators.

It would be nice to match their names at this early stage, preferably by renaming [GeneratedDllImport] to [DllImportGenerator].

Author: am11
Assignees: -
Labels:

area-System.Text.RegularExpressions, untriaged

Milestone: -

@jkotas
Copy link
Member

jkotas commented Nov 29, 2021

Interop source generator API proposal is tracked by #46822 (it is not in public surface yet). This issue can be used to track what if anything we want to change for RegEx.

@RussKie
Copy link
Member

RussKie commented Nov 29, 2021

How about calling the attribute [LibraryImport]?

More ideas: [Interop], [InteropImport], [PinvokeImport]

@MichalStrehovsky
Copy link
Member

As Jan suggested, let's leave interop discussion in #46822. I moved the interop suggestions into that issue.

@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Dec 15, 2021
@stephentoub stephentoub added this to the 7.0.0 milestone Apr 26, 2022
@joperezr joperezr modified the milestones: 7.0.0, Future Jul 18, 2022
@joperezr
Copy link
Member

Seems from the discussion that this is mainly to rename GeneratedDllImportAttribute right, and not so much for renaming RegexGeneratorAttribute?

@am11
Copy link
Member Author

am11 commented Jul 18, 2022

@joperezr, GeneratedDllImportAttribute was renamed to LibraryImportAttribute: #46822. This issue is tracking RegexGeneratorAttribute. Should we rename it to RegexAttribute?

RegexAttribute name is used in EntLib, but it is a rarely used library even with its NetCore port. If there is a chance of clash, it can be disambiguated using fully qualified names or with 'using' alias. It is the same situation as when System.Text.Rune was added, NStack.Core's System.Rune usages had to fully qualified the usages when upgrading projects to .NET 5 (one such project was gui.cs).

@joperezr
Copy link
Member

@terrajobst @stephentoub @bartonjs thoughts on the above comment? If we want to change it, now is likely our last chance of doing so.

@am11 am11 changed the title Reconcile naming: GeneratedDllImportAttribute vs. RegexGeneratorAttribute Reconcile naming: RegexGeneratorAttribute to RegexAttribute Jul 18, 2022
@stephentoub
Copy link
Member

RegexGeneratorAttribute is unique amongst LibraryImportAttribute, JsonSerializableAttribute, and LoggingMessageAttribute including "generator" in the name. I'd be ok with RegexAttribute.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2022
@teo-tsirpanis teo-tsirpanis added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 19, 2022
@stephentoub stephentoub added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 19, 2022
@terrajobst
Copy link
Member

terrajobst commented Jul 21, 2022

Video

  • We'd prefer GeneratedRegexAttribute
 namespace System.Text.RegularExpressions;

 [AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
-public partial class RegexGeneratorAttribute : Attribute
+public partial class GeneratedRegexAttribute : Attribute
 {
     // ....        
 }

@terrajobst terrajobst removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Jul 21, 2022
@terrajobst terrajobst added the api-approved API was approved in API review, it can be implemented label Jul 21, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label 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
api-approved API was approved in API review, it can be implemented area-System.Text.RegularExpressions blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants