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#matches? and String#matches? #8989

Merged
merged 4 commits into from
Apr 10, 2020

Conversation

makenowjust
Copy link
Contributor

Fixed #8950

It is another version of match method, which does not allocate MatchData when matched, instead it returns true or false. Avoiding allocation, it is efficient in fact.

#8950 (comment)

Fixed crystal-lang#8950

It is another version of `match` method, which does not allocate
`MatchData` when matched, instead it returns `true` or `false`.
Avoiding allocation, it is efficient in fact.

crystal-lang#8950 (comment)
# Calls `pcre_exec` C function, and handles returning value.
private def internal_matches?(str, byte_index, options, ovector, ovector_size)
ret = LibPCRE.exec(@re, @extra, str, str.bytesize, byte_index, (options | Options::NO_UTF8_CHECK), ovector, ovector_size)
# TODO: when `ret < -1`, it means PCRE error. It should handle correctly.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please keep TODO in this PR. Because, fixing this needs bigger diff than adding new feature IMO.

@j8r
Copy link
Contributor

j8r commented Apr 1, 2020

It is not ideal to have both #match and #matches? - they can be easily confused with non-existing #matches and match?.

@yxhuvud
Copy link
Contributor

yxhuvud commented Apr 2, 2020

There should probably be a discussion about naming though. Ruby defines a similar match? method returning a boolean. That clashes with the pretty strong naming convention in Crystal to add ? to the end of methods if they may return nil.

@Sija
Copy link
Contributor

Sija commented Apr 2, 2020

That clashes with the pretty strong naming convention in Crystal to add ? to the end of methods if they may return nil.

@yxhuvud ... but only when you have a non-nilable method version, like first & first? vs includes?

@makenowjust
Copy link
Contributor Author

makenowjust commented Apr 2, 2020

#matches is very unconventional in Crystal naming, so no one is confused with this naming.

#match? is... sure, it may exist. However we can notice that this naming looks odd. First, we know searching methods (like Enumerable#find, #bserarch or String#index) return nil when needle is not found in haystack, even though method name is not ended with ?. And, we expect #match should be such a method, then we cannot guess #match? behavior from its naming.

On the other hand, #matches? is clear in Crystal naming convention that it returns Bool value at least. Indeed, it is hard to notice this method does not create MatchData and it is efficient. However I don't know better name for this, and more descriptive name (like #match_without_matchdata_creation) seems not good to me.

As the above reason, I consider #matches? is reasonable naming.

@makenowjust
Copy link
Contributor Author

Documents of String and Regex's #match and #matches? are updated. They are now included a simple example showing $~ is not set after #matches?.

I think it is ready to merge. Please.

Copy link
Contributor

@Sija Sija left a comment

Choose a reason for hiding this comment

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

Wording suggestions.

src/regex.cr Outdated Show resolved Hide resolved
src/regex.cr Outdated Show resolved Hide resolved
src/regex.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
src/string.cr Outdated Show resolved Hide resolved
Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
@makenowjust
Copy link
Contributor Author

Thanks @Sija ❤️

Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>
@bcardiff bcardiff added this to the 0.35.0 milestone Apr 10, 2020
@bcardiff bcardiff merged commit 41f366c into crystal-lang:master Apr 10, 2020
@makenowjust makenowjust deleted the feat/regex/matches branch April 11, 2020 02:53
carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Apr 29, 2020
* Add `Regex#matches?` and `String#matches?`

Fixed crystal-lang#8950

It is another version of `match` method, which does not allocate
`MatchData` when matched, instead it returns `true` or `false`.
Avoiding allocation, it is efficient in fact.

crystal-lang#8950 (comment)

* Update documents of `{Regex, String}#matches?` and `#match`

* Improve document words in `#match` and `#matches?`

Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>

* Apply suggestions from code review

Co-Authored-By: Sijawusz Pur Rahnama <sija@sija.pl>

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
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.

Support match? : Bool for String and Regex
7 participants