Skip to content

add capture group name in comment - #63255

Merged
stephentoub merged 2 commits into
dotnet:mainfrom
pedrobsaila:62715
Jan 11, 2022
Merged

add capture group name in comment#63255
stephentoub merged 2 commits into
dotnet:mainfrom
pedrobsaila:62715

Conversation

@pedrobsaila

@pedrobsailapedrobsaila commented Jan 1, 2022

Copy link
Copy Markdown
Contributor

Fixes#62715

@ghostghost added area-System.Text.RegularExpressions community-contribution Indicates that the PR has been added by a community member labels Jan 1, 2022
@ghost

ghost commented Jan 1, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #62715

Author:pedrobsaila
Assignees:-
Labels:

area-System.Text.RegularExpressions

Milestone:-

@pedrobsailapedrobsaila mentioned this pull request Jan 1, 2022
Closed
@GSPP

GSPP commented Jan 2, 2022

Copy link
Copy Markdown

Can capture group names contain newline characters? That could break the code that's emitted.

@pedrobsaila

pedrobsaila commented Jan 2, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Can capture group names contain newline characters? That could break the code that's emitted.

Newline characters are not valid inside group name. If you try it inside a console app, you will get a RegexParseException with message : "Invalid pattern '[regex]' Invalid group name: Group names must begin with a word character."

stephentoub
stephentoub reviewed Jan 3, 2022
View reviewed changes
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
stephentoub
stephentoub reviewed Jan 3, 2022
View reviewed changes
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
stephentoub
stephentoub reviewed Jan 3, 2022
View reviewed changes
Comment threadsrc/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs Outdated
@danmoseley

danmoseley commented Jan 3, 2022

Copy link
Copy Markdown
Contributor

Newline characters are not valid inside group name. If you try it inside a console app, you will get a RegexParseException with message : "Invalid pattern '[regex]' Invalid group name: Group names must begin with a word character."

That message is wrong, it should say "Invalid pattern '[regex]' Invalid group name: Group names must only contain word characters.".

pedrobsailaand others added 2 commits January 11, 2022 07:10
@pedrobsaila@stephentoub
add capture group name in comment
4aaad01
@stephentoub
Some fixes for using capture group name
8f31872
- Pass correct arguments into GroupNameFromNumber from RegexCode
- Use capture group name for backreferences and backreference conditionals as well
- Only render the group number if there is no name
- Consistenly use the "capture group" suffix
- Clean up GroupNameFromNumber to avoid duplicate string.Empty fallback cases
- Fixes rendering of balancing groups
@stephentoub

stephentoub commented Jan 11, 2022

Copy link
Copy Markdown
Member

Thanks, @pedrobsaila. I rebased your branch and pushed a commit to fix up a few things. Should be good to go once CI passes.

@stephentoub
stephentoub merged commit bd9f1c4 into dotnet:mainJan 11, 2022
@pedrobsaila
pedrobsaila deleted the 62715 branch January 25, 2022 22:53
@ghostghost locked as resolved and limited conversation to collaborators Feb 25, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
+1 more reviewer
@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Text.RegularExpressionscommunity-contributionIndicates 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.

Regex source generator should include capture group name in comment

5 participants

@pedrobsaila@GSPP@danmoseley@stephentoub@joperezr