-
Notifications
You must be signed in to change notification settings - Fork 61
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
Add support for unicode properties in pattern, etc. #112
Add support for unicode properties in pattern, etc. #112
Conversation
I forgot about this for quite awhile, but noticed it in my queue today and took a stab. Please let me know if this will work. |
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.
Thanks!
src/jesse_lib.erl
Outdated
%% {@link https://www.erlang.org/doc/man/re.html#compile-2. re:compile/2} | ||
-spec re_options() -> list(). | ||
re_options() -> | ||
application:get_env(jesse, re_options, [ucp]). |
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.
When ucp
is enabled, shouldn't unicode
be added explicitly as well? It's a bit not clear from the documentation.
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 you're right that you need both:
1> re:run(<<"fōô"/utf8>>, "^\\w+$", [{capture, none}, ucp, unicode]).
match
2> re:run(<<"fōô"/utf8>>, "^\\w+$", [{capture, none}, ucp]).
nomatch
I will update the default and docs.
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.
that makes me think that it might be nice to have at least some test that showcases this feature
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 agree, I will write some eunit tests for these new utility functions.
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.
Done! The last commit (388cfa6) should address your concerns and suggestions. Thanks!
1160a9f
to
cb7ee7c
Compare
Resolves for-GET#104. Adds support for unicode properties "pattern" and "patternProperties" by adding `re_options` to the `jesse` application environment and making all uses of `re:run/2` consult it. This effectively changes the previously hard-coded `unicode` option to `ucp`, but allows restoring the previous behavior (for better performance) by setting `re_options` to `[unicode]` instead of `[ucp]`.
cb7ee7c
to
388cfa6
Compare
LGTM, thanks. Let's see if @andreineculau have any opinions |
I'll merge it on Tuesday 14th, if noone objects before that |
Resolves #104.
Adds support for unicode properties "pattern" and "patternProperties" by adding
re_options
to thejesse
application environment and making all uses ofre:run/2
consult it. This effectively changes the previously hard-codedunicode
option toucp
, but allows restoring the previous behavior (for better performance) by settingre_options
to[unicode]
instead of[ucp]
.