Coding style for regexps in oxidized

[robertcheramy]

Oxidized actually uses // as a regexp literal, wich is extensively used in the models:

class IOS < Oxidized::Model
# ...
  prompt /^([\w.@()-]+[#>]\s?)$/
  comment '! '
# ...
  cfg.gsub! /^(snmp-server community).*/, '\\1 <configuration removed>'

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: 1) Wrap the regexp in parenthesis

class IOS < Oxidized::Model
# ...
  prompt(/^([\w.@()-]+[#>]\s?)$/)
  comment '! '
# ...
  cfg.gsub!(/^(snmp-server community).*/, '\\1 <configuration removed>')

2) Use %r as a regexp literal:

class IOS < Oxidized::Model
# ...
  prompt %r{^([\w.@()-]+[#>]\s?)$}
  comment '! '
# ...
  cfg.gsub! %r{^(snmp-server community).*}, '\\1 <configuration removed>'

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:

• Style/RegexpLiteral (EnforcedStyle: slashes wich is the default)
• Lint/AmbiguousRegexpLiteral (Enabled: true)

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?

[ytti]

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.

[robertcheramy]

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'll have a look if we can disable the rubocop Lint/AmbiguousRegexpLiteral for the whole /lib/oxidized/model directory. If not, I will keep it disabled globally.

[robertcheramy]

I think PR #3263 is a balanced solution.