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
feat: add Regex() matcher #114
Conversation
8ff0dc9
to
9ec4913
Compare
Hi, I found this PR interesting---though I'm neutral about whether this feature should be added. If it's to be added, I wonder if we could make these two changes:
PS: I'm not an admin who can decide whether to merge this PR or not. |
Hi @merrett010 thanks for this PR. I'm not opposed to adding this feature if others aren't. However, I agree with both of @favonia's suggestions. While in most cases I would imagine the matcher will be used a single time - it wouldn't hurt to be on the safe side and compile the regular expression once. Matching |
Thanks both for your comments. I agree @favonia's suggestions are a nice addition. Have pushed up a new commit fyi @JacobOaks |
@merrett010 I made three suggestions:
I still remain neutral about whether this feature should be included. |
@favonia Thanks for reviewing, all seemed like good suggestions to me |
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.
Mostly looks good to me - just two couple small suggestions.
Cheers @JacobOaks, have seen to those now |
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.
LGTM!
Closes #113
Let me know if it's something you'd be interested in having - happy to make some tweaks if required