Add rubocop

[Evan-M]

Taking a stab at adding rubocop to the repo, as suggested in #1124

I've already addressed some of the simpler Cops that can utilize the --auto-correct option.

Each correction for a given cop was made in a separate commit, so to make reviewing easier (I hope).

The flow I've used was as follows:

1. Pick a Cop / set of violations in the .rubocop_todo.yml
2. Create a local branch with the Cop name
3. Make 1 or more updates to code, or move a todo item into the main .rubocop.yml config.
4. Merge the Cop branch back onto the add_rubocop branch with the --no-ff option.

This PR gets the offenses down to 652, but there is still more work to do!

[Evan-M]

Hold off on review for the moment.

[MaicolBen]

This looks good! I saw that you split the work with @dks17, great because this doesn't have it done at once. I hope in the future, we can add it to travis. When this is ready, just remove the "on hold" label.

I would squash the commits at the end because they are a lot, but we can do it at merge time.

Thanks @Evan-M !!

[Evan-M]

Yeah, 👍@dks17 for helping out with this!

I tried local 'feature/cop' branches with merge commits for each cop, but it doesn't look like the ref hierarchy made it up to github. In any event, I think I'll try @dks17's approach with the rest of this PR, and have a single commit for each cop fix.

Also, I know I've seen travis running the rubocop code, but it always feels like it blocks test from running. I know that codeclimate can utilize rubocop, so maybe that would work better? https://docs.codeclimate.com/docs/advanced-configuration

And I agree that squashing these commits at the end makes sense, but lets get all the commits reviewed beforehand!

[Evan-M]

I merged #1126, but then ran into all kinds of rebase and/or merge duplicates and other silliness.

If an attempt to make sure that this PR ends up in a reviewable and mergeable state, I cherry-picked the commits that @dks17 made in #1126 back to this branch, squashed all my previous merge commits (such that there is one commit per set of cop violation fixes), and did some --fixup commits + git filter-branch revisionist history magic.

This of course required a force-push. So if anyone else has checked-out or forked this branch, I apologize in advance! To get back to valid state, rename your existing checkout/fork, re-checkout or re-fork a clean copy of the updated branch, and rebase your changes against the new copy.

[dks17]

I have just pull master into rubocop branch.
Almost each new PR in master branch brings some conflicts and offences. With every PR it seems we will never overtake master branch.
Should be better merge rubocop branch into master now?

[MaicolBen]

@dks17 Agreed, we don't have frequent PRs merged lately, but this could take a while to be finished.

Rebase it, we make sure Travis passes and we will merge it (squashing)

[dks17]

@Evan-M
I have got many warnings in console like these:

Warning: unrecognized parameter Metrics/BlockLength:inherit_mode found in .rubocop.yml
Warning: unrecognized parameter Metrics/LineLength:inherit_mode found in .rubocop.yml

Do you thing these configuration parameters were set properly?

Metrics/BlockLength:
  inherit_mode:
    merge:
      - Exclude
      - ExcludedMethod

Metrics/LineLength:
  inherit_mode:
    merge:
      - Exclude

It seems there is no effect if these properties are commented.

[Evan-M]

That happens when there are no inherited settings to merge with from an
included file (in this case, there are no Excludes for the
Metrics/BlockLength nor the
Metrics/LineLength cops inherited from .rubocop_todo.yml).

If you comment out the inherit_from: .rubocop_todo.yml at the top of the
.rubocop.yml file, you will see more warnings where there is nothing to
merge.

[krzysiek1507]

I'm not a big fan of rubocop -a because, to be honest, does anybody check every single line here? It's too much to check and most people just scroll it after 10 files. I prefer to do it step by step, e.g. fix hash style, fix indentation and so on, so we can be sure that we don't break anything (e.g, change and to &&).

[dks17]

I did not use auto correction (manually only) then ran the tests.

[Evan-M]

I've been using the -a option in conjunction with --only <COP_NAME>. Then I review all the changes via git add --patch, and make sure the tests pass before committing the changes.

I also frequently use rubocop --only <COP_NAME> --format worst to check the violations before running with -a option.

¯\_(ツ)_/¯

@krzysiek1507 To your point though, running rubocop -a against all cops, kinda feels like biting off more than you could chew.

[krzysiek1507]

I saw a lot of pull requests with rubocop -a, green tests and new bugs introduced.

In this case, it's better but it is still hard to review it because of 2.5k additions and 1.9 deletions.

[krzysiek1507]

We can always split this PR into smaller by cherrypicking.

[krzysiek1507]

You don't need to freeze strings if there is # frozen_string_literal: true pragma.

[MaicolBen]

@Evan-M Is this still on hold? because we can merge after you fix @krzysiek1507 comment, and you continue it in another PR, so it doesn't keep growing

[MaicolBen]

@krzysiek1507 @Evan-M I reviewed the PR and it looks good, as I don't see more activity from the author and I don't want to lose this work, I will merge it. About frozen_string_literal, most of the files have it, so I don't see bad adding it