-
Notifications
You must be signed in to change notification settings - Fork 918
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
Coding style for regexps in oxidized #3261
Comments
I think the warning can be disabled, at least in 3.4 and forward. I'm not going to fight against any chance, but personally I feel like it is most readable as it is. More so, I kind of view the models more as DSL, and less as ruby, and likely would have more strict desire to conform to ruby in non-model files, but for models I think many of our authors don't even realise they are writing ruby. |
I've looked into the ruby source - ruby always chooses this is a regexp when there is an ambiguity with a division: https://github.com/ruby/ruby/blob/3db2782748e0753f0e4b5c10e6837e0609c5ad1b/parse.y#L11159 It seems to me the warning can only be disabled globally, for the unit tests here in the /Rakefile As I also like the DSL-style of the models, so I would prefer to disable the warnings for the unit tests by default. |
I think PR #3263 is a balanced solution. |
Oxidized actually uses // as a regexp literal, wich is extensively used in the models:
This is a problem as it very often produces a
warning: ambiguity between regexp and two divisions: wrap regexp in parentheses or add a space after '/' operator
. The warnings are not visible on the console (I don't know why), but become visible when writing a model unit test (wich I am currently working on).There are two solutions to solve the warnings:
Ruby style says "Use %r only for regular expressions matching at least one / character.", so solution 1 seems the better solution to me, although I liked specifying the prompt without parenthesis in the models.
I propose to activate rubocop cops to enforce this:
Runing rubocop -a will autocorrect both cops, and should have a consistent solution. Note - regexp without ambiguity will still be coded without parenthesis.
@ytti - do you have an opinion on this?
The text was updated successfully, but these errors were encountered: