Improve handling of word boundaries, fix #146 #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
My internship with Jane Street ends today, so future communication on this PR should be redirected to @dwang20151005. |
|
Sounds good to me! Could you add tests to exercise it ? Also, I suggest squashing the commits (I'll do it when merging otherwise). (cc @dwang20151005 ?) |
And a CHANGES entry as well please. |
ericmarkmartin
force-pushed
the
patch-1
branch
from
891a474
to
527ee9b
Compare
In addition to the minimal fix suggested by Vouillon in ocaml#146, this PR allows `Category.inexistant` characters (i.e., `bos` and `eos`) in the following places: 1. before `bow` 2. after `eow` 3. on both sides of `not_boundary` (but not one side alone) That minimal fix would still give an unintuitive and unconventional interpretation of word boundaries where `bos` and `eos`... 1. are accepted on both sides of a word boundary (`bow` or `eow`) 2. are not accepted on either side of a `not_boundary` With the minimal fix, `bow` and `eow` matches would not need to have a word between them. E.g., 1. in `""`, both `bow` and `eow` would match at position 0 with nothing between them 2. in `"#"`, `bow` would match before the `#` and `eow` after the `#`, even though `#` is not a word character. In contrast, the convention in other regex libraries is that `bos` and `eos` are not words: 1. [Ruby](https://rubular.com/r/7jdsGlU3LOPrX2) 2. [Python](https://pythex.org/?regex=%5Cb&test_string=%23&ignorecase=0&multiline=0&dotall=0&verbose=0) 3. [re2 syntax reference](https://github.com/google/re2/wiki/Syntax). 4. PCRE: can't permalink, but you can check on https://regexpal.com . 5. JavaScript: ditto, https://regexpal.com . This PR implements this more conventional behavior. Jane Street is migrating from `Re2` to `Re` by writing a wrapper that implements the `Re2` interface on top of `Re`. In most cases where `Re2` disagrees with `Re`, we either worked around it in the wrapper or let the wrapper diverge from real `Re2`. This seems like a case where we can't work around it, and where `Re2` has the more intuitive and conventional behavior. test(test_re): add regression tests for bow, eow, word, not_boundary docs(CHANGES): add CHANGES entry
ericmarkmartin
force-pushed
the
patch-1
branch
from
527ee9b
to
7041809
Compare
|
I've added regression tests and a CHANGES entry, and squashed the commits. |
|
Thanks |
rgrinberg
added a commit
to rgrinberg/opam-repository
that referenced
this pull request
Apr 27, 2022
CHANGES: * Improve handling of word boundaries (ocaml/ocaml-re#179)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed
In addition to the minimal fix suggested by Vouillon in #146, this PR
allows
Category.inexistantcharacters (i.e.,bosandeos) in thefollowing places:
boweownot_boundary(but not one side alone)Justification of change
That minimal fix would still give an unintuitive and unconventional
interpretation of word boundaries where
bosandeos...boworeow)not_boundaryWith the minimal fix,
bowandeowmatches would not need to have aword between them. E.g.,
"", bothbowandeowwould match at position 0 withnothing between them
"#",bowwould match before the#andeowafter the#,even though
#is not a word character.In contrast, the convention in other regex libraries is that
bosandeosare not words:This PR implements this more conventional behavior.
Use case
Jane Street is migrating from
Re2toReby writing a wrapper thatimplements the
Re2interface on top ofRe.In most cases where
Re2disagrees withRe, we either worked aroundit in the wrapper or let the wrapper diverge from real
Re2.This seems like a case where we can't work around it, and where
Re2has the more intuitive and conventional behavior.