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.

Sign up for GitHub

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

Jump to bottom

Implement #match! for Regex #13285

Merged
merged 18 commits into from
Jun 6, 2023
Merged

Implement #match! for Regex #13285

merged 18 commits into from
Jun 6, 2023

Conversation

devnote-dev
Copy link
Contributor

This PR implements String#match! and Regex#match! (the former using the latter) and replaces all the Regex::MatchData examples using #not_nil! with #match!. Closes #13119 and #13120.

@straight-shoota
Copy link
Member

Looks good 👍
But I think the exception raised when there is no match should be more specific than Exception. Maybe we could keep NilAssertionError from .not_nil!, but that would feel odd. Regex::Error might be a more suitable candidate?

@devnote-dev
Copy link
Contributor Author

devnote-dev commented Apr 5, 2023 •
edited
Loading

I went with Exception because #not_nil! was too ambiguous, I don't think Regex::Error exists though? If not then I can add it in the regex file.

Found it, it was at the top 😅

Sija
Sija reviewed Apr 5, 2023
View reviewed changes
src/regex.cr Outdated Show resolved Hide resolved
Sija
Sija reviewed Apr 5, 2023
View reviewed changes
spec/std/regex_spec.cr Outdated Show resolved Hide resolved
spec/std/regex_spec.cr Outdated Show resolved Hide resolved
devnote-dev and others added 2 commits April 5, 2023 13:56
@devnote-dev @Sija
Update spec/std/regex_spec.cr
a796411 
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
@devnote-dev @Sija
Update spec/std/regex_spec.cr
a823ae0 
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
HertzDevil
HertzDevil reviewed Apr 5, 2023
View reviewed changes
src/regex/match_data.cr Outdated Show resolved Hide resolved
Sija
Sija reviewed Apr 5, 2023
View reviewed changes
src/regex.cr Outdated Show resolved Hide resolved
straight-shoota
straight-shoota reviewed Apr 21, 2023
View reviewed changes
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Specs for String#match! would be nice.
Besides that I just have some small suggestions for doc improvements.

src/regex.cr Outdated Show resolved Hide resolved
src/regex.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
@devnote-dev
Copy link
Contributor Author

Specs for String#match! would be nice.

Is that necessary? String#match! calls Regex#match! so there would be no difference in specs.

@straight-shoota
Copy link
Member

straight-shoota commented Apr 21, 2023 •
edited
Loading

Yes, that's currently the implementation. And chances are that it's gonna stay that way more or less. But this method isn't just a simple delegation due to the assignment of $~. And there could be some future refactoring with break potential. It's best to have specs to cover at least the basic expected behaviour.

@oprypin oprypin changed the title feat: implement #match! for Regex Implement #match! for Regex May 25, 2023
@beta-ziliani beta-ziliani added this to the 1.9.0 milestone Jun 3, 2023
@straight-shoota straight-shoota merged commit 1b5dcee into crystal-lang:master Jun 6, 2023
@devnote-dev devnote-dev deleted the string-match branch June 6, 2023 09:16
@stufro stufro mentioned this pull request Jul 23, 2023
Closed
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
@devnote-dev @Sija
@straight-shoota @Blacksmoke16
Implement #match! for Regex (crystal-lang#13285)
089d9cc 
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
Co-authored-by: Johannes Müller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HertzDevil HertzDevil HertzDevil left review comments

@Sija Sija Sija left review comments

@beta-ziliani beta-ziliani beta-ziliani approved these changes

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib:text
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.

Add String#match!
5 participants
@devnote-dev @straight-shoota @Sija @HertzDevil @beta-ziliani