-
Notifications
You must be signed in to change notification settings - Fork 8
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
Increase regex readability #391
Conversation
This PR makes all regex patterns that contain escapes raw String literals, so that you do not have to mentally double unescape to understand the pattern. It also removes unnecessary start- and end-of-input anchors for regex that are anyway only used to match entire inputs. |
c5c22ca
to
0656820
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.
Hi, @Vampire. Thank you very much for the contribution
If the anchors should stay, they should at least be changed to always be used, currently in most cases it was like ^foo|bar|baz$ instead of ^(?:foo|bar|baz)$
I think it would be better to correct the patterns so the anchors are used correctly - just a precaution in case an incorrect method on Regex is used (that does not match the entire string against the pattern).
...commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/constructor/json/ConstructYamlBinary.kt
Outdated
Show resolved
Hide resolved
src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/common/Anchor.kt
Show resolved
Hide resolved
Sure, I hope I didn't break it now. :-D |
44e5abe
to
c280b2d
Compare
...commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/constructor/json/ConstructYamlBinary.kt
Outdated
Show resolved
Hide resolved
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.
src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/resolver/CoreScalarResolver.kt
Show resolved
Hide resolved
Generally looks sane, but I'm worried a bit about compatibility of the regexes across all targets. AFAIK, there are some minor discrepancies, and our tests aren't fully multiplatform yet (even if they are, not sure if coverage is good enough). What I propose to do is to go to the modified file where we use something suspicious, like the non-capturing groups, and checking if the tests exercising the regexes are KMP already. If not, I'd recommend porting them first. |
src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/resolver/CoreScalarResolver.kt
Show resolved
Hide resolved
src/commonMain/kotlin/it/krzeminski/snakeyaml/engine/kmp/resolver/JsonScalarResolver.kt
Outdated
Show resolved
Hide resolved
Did you mistype and mean you are worried? |
From a quick look I'd say Native and WASM is an own Regex implementation also supporting non-capturing groups. |
Yes,sorry! Yeah, things should probably work fine, although as a rule of thumb, if we touch some code, I'd prefer to have coverage for all targets. I can migrate the two tests tomorrow (they're pretty simple), and this way we'll increase confidence regarding correctness of this change. |
5fb36d3
to
edb7529
Compare
No description provided.