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

Add Regex#capture_count #9746

Merged
merged 1 commit into from
Dec 6, 2020

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Sep 14, 2020

Adds missing method counterpart to Regex#name_table.

@Sija
Copy link
Contributor Author

Sija commented Oct 20, 2020

Friendly ping for a review /cc @bcardiff @asterite @jhass

@asterite
Copy link
Member

Why is this needed?

@Sija
Copy link
Contributor Author

Sija commented Oct 20, 2020

@asterite It's needed in cases where you need to introspect the given regex, say for generating default values for all/some of the capture groups. Obviously not a very commonly used feature, but nevertheless practical and complementary to above-mentioned Regex#name_table, plus it's the only way to obtain this information and so it feels weird to monkey-patch already supported PCRE feature.

@asterite
Copy link
Member

Thanks! I was just curious because it's not there in Ruby, so I wonder how people managed to workaround that.

@Sija
Copy link
Contributor Author

Sija commented Dec 3, 2020

Can this get a 2nd approval?

@straight-shoota straight-shoota added this to the 1.0.0 milestone Dec 3, 2020
@asterite
Copy link
Member

asterite commented Dec 3, 2020

What is this in Ruby?

@Sija
Copy link
Contributor Author

Sija commented Dec 3, 2020

@asterite I don't there is a such thing.

@Fryguy
Copy link
Contributor

Fryguy commented Dec 3, 2020

Possibly because Ruby uses oniguruma and from what I can tell it doesn't have this method either?

EDIT: Seems this is closest to onig_number_of_captures ?

@straight-shoota straight-shoota merged commit 78e509f into crystal-lang:master Dec 6, 2020
@straight-shoota
Copy link
Member

Thanks @Sija

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants