-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Extract internal Regex API for PCRE backend #12802
Extract internal Regex API for PCRE backend #12802
Conversation
42836ff
to
dd12f5c
Compare
So there was actually a bug in the original refactor that didn't get caught by the spec suite, despite its huge expansion. I added some missing test cases in #12804 (also first commit in this branch). |
c087ce6
to
ca016bd
Compare
4d50cf9
to
a3383de
Compare
a3383de
to
2827c9c
Compare
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.
Nothing worth blocking 👍
src/regex/pcre.cr
Outdated
# :nodoc: | ||
module Regex::PCRE | ||
# :nodoc: | ||
def initialize(*, _source source, _options @options) |
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.
Why not make this one private too? Also, wouldn't it make sense to change its name, and avoid the renaming of arguments?
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.
Yeah, I think private
should work. I think the nodoc is an artifact from an earlier setup where private
wouldn't have worked.
Changing the name would make this not be a constructor which causes problems with ivar initialization.
This patch extracts code that relates directly to implementation details of the libpcre backend from
Regex
andRegex::MatchData
and puts it into a moduleRegex::PCRE
.This abstraction is then to be used for an alternative engine using PCRE2 (#12790).
The first two commits are an separate change submitted in #12810.Commit
three and fourone and two are general refactors which are useful on their own and make the extraction easier.