Rubocop

[pushcx]

I'm adding rubocop to enforce style issues to save human attention (and patience) in code reviews. The default config found 4,849 offenses and this branch will be merged when there are zero.

Like everybody I have opinions on style issues, but my preferred style for any codebase is whatever the codebase is already doing. With that in mind, I'm configuring cops to enforce the common usage and then tweaking outliers to match, eg. 988aa22 and fda61bb.

If you have opinions on how best to paint this bikeshed, this PR is the place to speak up. Once this is merged I'd like to change style config as rarely as possible because it's distracting churn when reading through history during bughunts. Speak now or forever hold your peace or, in the future, make a compelling, metrics-driven argument that a style change will materially improve code quality.

The current 3,382 offenses may seem like a lot, but if you sort by offense count you'll of course see a pareto-like distribution of 135 cops, so this PR hopefully won't linger for months. rubocop | grep -o '[A-Z][^/]*/[A-Z][^ ]*' | sort | uniq -c | sort -rn | head -n 20

TODO: run rubocop as part of the test suite so humans see it easily and Travis fails the build.

[talklittle]

👍 makes sense

On a related note, given the automated checks being added to the repo, maybe maintainers could refrain from committing non-critical changes directly to master in the future, so they'll be subject to automated checks on their pull requests as well? 😉

[hmadison]

This looks good to me.

[jcs]

I don't really care about this code anymore so do whatever you want, but:

Like everybody I have opinions on style issues, but my preferred style for any codebase is whatever the codebase is already doing.

I think I was/am pretty consistent in my code style but most of this diff changes the style, not the code to conform to the existing style as you said.

-    new_tags.keep_if {|t| t.valid_for? @user }
+    new_tags.keep_if { |t| t.valid_for? @user }

All of the existing code uses {|, so how is { | considered the existing style?

$ fgrep -r -- '{|' app | wc -l 
      51
$ fgrep -r -- '{ |' app | wc -l
       0

Similarly for spaces around brackets in array declarations.

[pushcx]

I didn't notice the codebase was consistent. I haven't seen another do it so it looks odd to me. I put no thought into that one, I autocorrected it and moved on.

Edit: Spaces around brackets was pretty evenly split: 130 arrays without space, 142 with spaces. I considered disabling it, but I think it's come up in a PR so I went with the rubocop default.

Double edit: As long as someone's reading these... the existing style Layout/MultilineOperationIndentation was pretty consistent as 'the thing vim indenting does", but that doesn't match any cop options. Same with Layout/AlignParameters, but also indenting was more consistent with the cops around aligning hashes, arrays, parameters, and Layout/DotPosition.

As a warning to anyone paying attention to this PR: I will probably do a rebase fixup and push --force to revert Layout/SpaceInsideBlockBraces in 1ea897a to have less churn in the history of the main branch. I've been thinking merge rather than squash because there's some commit messages, but maybe no one will ever care about those on style and one big commit is less awful? Thoughts?

[pushcx]

As described, I've edited 1ea897a into b1f83f5 to remove the space between block curly braces and arguments.

[pushcx]

OK, no cops complaining and the merge conflicts are trivial. I'll merge Wednesday morning, if anyone wants to express an opinion before then.

[sean-public]

Since this touches so many files, I imagine anyone working in a branch will need to rebase (perhaps even painfully). Is there any such work going on and should anyone be notified in particular to make sure they see this mega-merge incoming?

[talklittle]

Good point, @sean-public -- would be nice to get outstanding PRs merged first, so the authors won't be forced to conform to style checks that didn't exist when they initially created the PRs.

[pushcx]

I merged the open PRs that were ready to go.

[pushcx]

OK, and merged this branch down, too. (I'm adding it to the test suite + travis in a separate commit because of hassles around the bumped deps.)