-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make startswith
, endswith
work with Regex
#29790
Conversation
From suggestion by @ScottPJones, I changed the methods to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I like the version with anchor options very much.
The Windows 32 build is failing over something related to profiling. Can't quite figure out if it is related to this PR. 🤷♀️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some simple docstrings?
Isn't the weirdness of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to address the 100x slowdown over occursin
.
Co-Authored-By: dalum <17059936+dalum@users.noreply.github.com>
Co-Authored-By: dalum <17059936+dalum@users.noreply.github.com>
With the latest changes thanks to @chethega, this version is now only 1.3x slower than using |
Error messages during bootstrap are obscure. This bit meant that it failed to interpolate a
|
@StefanKarpinski @fredrikekre Ping! Tests are passing. I think it's ready now. 🎉 |
The rationale behind this PR is two-fold.
The first is motivated by a real use–case of checking the file extension of a file, where something like
occursin(r"\.skf$"i, filename)
seems to employ a "reverse query" logic to me that reduces readability: does this pattern, which must be at the end, occur infilename
? Compared with:endswith(filename, r"\.skf"i)
: doesfilename
have this pattern at the end?The second is an argument about genericity, where a function such as
occurs_not_endswith(pat, s) = occursin(pat, s) && !endswith(s, pat)
presently wouldn't work forpat::Regex
. I don't have a use for this, but it seems to me that it should work.As an aside, the implementation here allows use such asendswith("abc\ndef", r"c"m)
to check if any line in the string ends with"c"
.The implementation of course has the slight overhead that if passed an already compiled🙂Regex
, this will generate and compile a newRegex
on every call, but I would think that the cases where this is a real issue are not that frequent. And users who need this little bit of extra speed could probably be expected to understand the caveats of using a convenience function like the ones introduced here.