Rubocop too overzealous?

[jyaworski]

Hello:

I've been working on the puppet-rundeck modulesync changes at voxpupuli/puppet-rundeck#134

It looks like it's failing on rubocop, but a lot of it is very benign.

1. Long lines
2. and vs &&
3. Nested functions
   etc

One of the main things I'm running into is:

rubocop/rubocop#1238

Can we tone down Rubocop's aggression?

[jyaworski]

Also here:

rubocop/rubocop#947

[jyaworski]

One possible solution is to use --only-guide-cops as documented in rubocop/rubocop#1454

[igalic]

@jyaworski that seems like a very good idea, and is probably very much less… opinionated.!

[igalic]

@jyaworski can you add pr that changes our config to that?

[mkrakowitzer]

Can I suggest using .rubocop.yml from garethr/puppet-module-skeleton as a base?

https://github.com/garethr/puppet-module-skeleton/blob/master/skeleton/.rubocop.yml

[igalic]

there are a couple of those i don't entirely agree with… such as:

Style/RegexpLiteral:
  Enabled: false

that's literally the reason i started this whole thing ;)

[jyaworski]

One solution is to add --fail-level and set it to a sane default; say 'E'. Maybe add a parameter to .sync.yml to toggle it per-project if you want? How does that sound?

joseph.yaworski@jyaworski-mbpr ~/puppet-rundeck (git)-[modulesync] % rubocop
...
25 files inspected, 121 offenses detected
joseph.yaworski@jyaworski-mbpr ~/puppet-rundeck (git)-[modulesync] % echo $?                                                                                                                                                             :(
1

joseph.yaworski@jyaworski-mbpr ~/puppet-rundeck (git)-[modulesync] % rubocop --fail-level=E
...
25 files inspected, 121 offenses detected
joseph.yaworski@jyaworski-mbpr ~/puppet-rundeck (git)-[modulesync] % echo $?                                                                                                                                                             
0

[mkrakowitzer]

OK so my initial thoughts on trying to get my spec tests reformatted to meet the current requirements is that I will still be reformatting perfectly working code in about 2 weeks from now, and this is only one project! I have literally spent an entire evening formatting text and I am only about 50% done. Rubocops autocorrect is a disaster, so that did not even help.

We need a more sane config file in order for any module to make rubocop a viable option or force it to exit 0 as per @jyaworski previous comment.

[igalic]

Agreed.
the defaults are impossible to comply with, especially in rspec tests.

[daenney]

I don't think long lines or and vs && is pretty benign. Long lines are hard to read and the longer they get the more complicated expressions they usually contain.

&& vs and are pretty similar in Ruby but their difference is in binding strength / operator precedence so we should care about those. The style guide is pretty clear on this too:

Use &&/|| for boolean expressions, and/or for control flow. (Rule of thumb: If you have to use outer parentheses, you are using the wrong operators.)

We can certainly tone them down a bit but just throwing out parts from the style guide we find inconvenient is not something I'm willing to support. Theres should be good reasons and arguments for the exceptions we're going to make.

[igalic]

@daenney i also agree with that, however, rubocop's default auto_correct behaviour is awful…

[daenney]

Then don't autocorrect/use the fixers, just let it complain about it and fix it yourself 😄. There's a few we can disable but from the presented use cases I only find nested functions potentially troublesome. But nested functions are also bad.

[igalic]

fixing thousands of lines code isn't really my idea of fun :P

[daenney]

It's not thousands of lines, it's one module at a time and not all modules, far from all, have native types and providers. And new code can start out directly by adhering to this. There's also no need to go full on zealot on it and fix everything now now now.

[igalic]

when i say thousands of lines of code, i specifically mean rspec tests.
but yes, you are right.

[danzilio]

Definitely agree with @daenney on and vs. &&. Also, don't forget about control comments for exceptions. Sometimes breaking with the style guide on a single line makes sense for readability or maintainability. You can disable a cop for a single line by adding a trailing control comment like this: # rubocop:disable RULENAME. This should obviously be used judiciously, but we shouldn't be pedantic.

[igalic]

after merging #21 i think we've got some better fit…

[igalic]

now, if only we had some kind of trigger that would run msync update for us…

[jyaworski]

I just resynced voxpupuli/puppet-rundeck#139 and I'm getting different errors. Still looking into them.

[jyaworski]

All right. Lots of refactoring later and I'm back to a smaller number of rubocop errors.

@daenney and @danzilio: I'm not super well-versed in Ruby, so maybe I spoke out of naivete with the and/&& thing. Long lines I understand the frustration as well. However, I share @igalic's general frustration surrounding rspec tests; that's where all of that PR I mentioned is failing, and some of them are non-trivial to fix. Do we really want to lint spec files in that case?

[daenney]

That is a good question. I'm not convinced the spec tests have to adhere to all the Rubocop madness. It might be worth only loading rubocop-rspec for the spec/ directory and a few basic other cops.

[mkrakowitzer]

Personally I think it makes the spec tests harder to read: https://github.com/puppet-community/puppet-stash/pull/99/files

[jyaworski]

All right; I've turned off a few cops in #24.

[igalic]

@mkrakowitzer i can't even tell the difference anymore _
@jyaworski merged

[logicminds]

We need an exclusion for these kinds of errors. I don't agree with rubocop

lib/puppet/parser/functions/default_content.rb:10:1: C: Align ) with (.
) do |args|
^
lib/puppet/parser/functions/dump_args.rb:11:1: C: Align ) with (.
) do |args|

[daenney]

Well, just because one person doesn't agree is not a good enough reason to exclude. Why do you disagree and perhaps share a few screenshots of code that is more legible in your case than what rubocop recommends?

[igalic]

i'm pretty and sure i've excluded those ;)
@daenney they make most of our rspec code completely unreadable

[logicminds]

@daenney this is the default way of defining puppet functions. example: logicminds/puppet-extlib@fc46c63#diff-e24a4c8f84f47be4ba90e4572ed66e18R11

Every puppet function in puppet core is defined this way.

[jyaworski]

I just linked from a PR where it's complaining about module comments. Definitely not something worth failing on.

[igalic]

@jyaworski any idea what needs to change to make it pass?

[jyaworski]

@igalic yes.

#35