Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[Api-addition] Regex group should provide a Name property.#1384

Merged
Priya91 merged 2 commits intodotnet:futurefrom
Priya91:apiaddition
Apr 14, 2015
Merged

[Api-addition] Regex group should provide a Name property.#1384
Priya91 merged 2 commits intodotnet:futurefrom
Priya91:apiaddition

Conversation

@Priya91
Copy link
Copy Markdown
Contributor

@Priya91 Priya91 commented Apr 11, 2015

API review was done internally. closes #1051
cc @tarekgh @terrajobst @joshfree

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would make this field readonly. The same for the _caps/_capcount fields.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, will make _name and _caps readonly. _capcount is set in Match, which derives from Group.

@weshaggard
Copy link
Copy Markdown
Member

Beyond the couple of comments it looks good to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

are we always going to have "0" name for the group with no specified name? what we used to return from GetGroupNames before this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, for the unspecified names, we return the index of the numbered group in the regex, as in 0,1,2. Here this Match constructor is used for initilaizing the objects, the base Group constructor, is passing 0 (number of captures), new int[2] for the start and len of the default capture, text = the whole text, hence passing "0" the default group number for the whole successful capture.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thanks. LGTM

@Priya91
Copy link
Copy Markdown
Contributor Author

Priya91 commented Apr 14, 2015

@weshaggard @tarekgh : Thanks for the review. Merging this.

Priya91 added a commit that referenced this pull request Apr 14, 2015
[Api-addition] Regex group should provide a Name property.
@Priya91 Priya91 merged commit 1e2364c into dotnet:future Apr 14, 2015
@Priya91 Priya91 deleted the apiaddition branch April 14, 2015 19:38
@karelz karelz modified the milestone: Future Jan 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants