Sensible configuration for rubocop

[fosterfarrell9]

The aim of this PR is to figure out a sensible configuration for rubocop in our project. Until now, we just used the rubocop config file of Rails. Looking at the details of what happens there, you see that many of these settings do not make sense for us.
In a first attempt to improve this, I incorporated Shopify's rubocop settings which are well documented and made some minor twists for settings where we have (until now) used different defaults. Note that in the shopify settings all Metrics/ methods of rubocop are disabled.
I put this first attempt up for discussion. If the general approach is considered okay, we might discuss where we deviate from shopify's config.

[fosterfarrell9]

This is now ready for review. I added a Style Guide Page in the wiki.

[Splines]

Some comments.

Thanks for the Wiki entry. I changed the link there to point to the webpage that shows the rendered markdown files (and not just the GitHub Markdown which does not support many things they are using, e.g. colors for headers, nice code formatting and so on).

[Splines]

Of course, this limit is highly controversial.

I strongly recommend 80 characters as a soft limit and we should include that in our style guide, e.g. in the Wiki. In VSCode you can also display a grey border at 80 chars.

However, in my past dev experience, I remember lines that were slightly longer than 80 chars and I was really upset that a linter forced me to break a statement to two lines. Sometimes this can actually make a statement less readable (given that the line was just a bit above 80 chars and not something like 120 chars or even more).

This is probably why they even got rid of the 80-column warning in the Linux kernel here. Instead, they raised the limit to 100 chars which I think is more reasonable as a hard limit.

[Splines]

For a future PR that aims to align all Ruby files in our entire codebase (715 Ruby files) to the new RuboCop style guides, this could be a very useful comment.

[Splines]

What about Style/ClassMethodsDefinitions, do we really want that?

It will enforce this affecting us 132 times in our codebase. I'd rather go with EnforcedStyle: def_self.

[fosterfarrell9]

Agree and done.

[fosterfarrell9]

I also updated the wiki page correspondingly.

[Splines]

Ok thanks. I wonder: if we add more custom things to the .rubocop.yml file, maybe it's a bit tedious to keep it synced with the Wiki? Except for the link to the style guide, I do not see the benefit of listing every change we make in the Wiki.

One could just open the .rubocop.yml file (maybe link to that in the Wiki?) or even run rubocop on the local code changes to directly see where one violates our styles. In the best case, the plugin for VSCode also adds blue squiggly lines to indicate style violations. In my setup, it even autocorrects these (if the autocorrections are safe) whenever I save the file.

If you argue that the Wiki is a place where we could add additional comments for why we chose to do something, we could also have that in the Details section.

[Splines]

Shopify also sets CountKeywordArgs to false which enforces this behavior. I prefer to set it to true as is the RuboCop default.

[Splines]

Shopify disabled Lint/UnreachableLoop. I vote for having it enabled.

[Splines]

Style/CaseLikeIf is disabled in Shopify. What do you think about adding a Severity: refactor to it?

[Splines]

What's your opinion on Style/GuardClause which is disabled in Shopify. I don't have a strong opinion on this but remember you once suggested to use unless, so you probably want it enabled?

[Splines]

I strongly vote to enable the Style/DoubleNegation cop.

[Splines]

Instead of disabling Style/EmptyMethod, we should enforce our preferred style, which is EnforcedStyle: expanded.

[Splines]

I strongly suggest enabling Style/NegatedUnless, otherwise you have to write down your own truth table to find out when the if statement is true.

[Splines]

I strongly suggest enabling Style/NegatedIfElseCondition.

[Splines]

I suggest enabling the cop Style/YodaCondition with the default EnforcedStyle.

[Splines]

I strongly suggest enabling the cop Style/TrailingMethodEndStatement. Why would one disable that? I see no scenario where that could be a useful thing to do.

[Splines]

I strongly suggest enabling the cop Style/TrailingMethodEndStatement.

[Splines]

I prefer to have Style/SwapValues enabled as this makes the intention of what is really done in the code clearer.

[Splines]

I suggest not disabling any security-related RuboCop cop like Shopify does. This is since I don't feel like I'm in the expert position to judge if we don't need this, so I'd prefer having this as an error that signifies to me I really have to watch out and should probably use a different way to do things (since smart people already figured this could be a potential security issue).

I even suggest writing the following to enable all security-related Rubocop cops.

Security:
  Enabled: true

[Splines]

I don't see a use-case for self-assignment, so I'd suggest to enable the cop Lint/SelfAssignment.

[Splines]

I suggest enabling the cop Lint/EmptyBlock.

[Splines]

I suggest enabling the cop Lint/DuplicateRequire.

[Splines]

To be honest: meanwhile I feel that for almost every cop they disabled at Shopify, I want to enable it again and disable it only if we encounter a use-case to opt-out of it in our own code base. Maybe a better strategy would be to go with the Rubocop defaults; it has many cops (which I suggested to enable in this PR) enabled by default.

I mean we could still disable some things such as metric-related stuff (e.g. too long methods etc.). In the end, Rubocop should serve us and not vice-versa.

When starting fresh, it might be very helpful to run the Rubocop autocorrect locally, see what things it proposes to change. One can then skim through the changes and if we don't like a change, we can deactivate it.

The command I use is:

bundle exec rubocop --autocorrect --disable-uncorrectable

Then take a look at the changes and also search for rubocop:todo for things that could not be autocorrected. Still: it is almost certain that the config we come up with in this PR will not be the final one and we will have to opt-in or opt-out of some cops sooner or later, but then we know exactly why we've done it.

Very useful for us is also this section.

---

Note I've reduced our existing .rubocop.yml file to just 81 lines (including a long comment) here. See the commit message for the explanation (and this section).

[Splines]

@fosterfarrell9 I've tried to reduce our existing .rubocop.yml file by using the fact that RuboCop defaults are already imported by default, so we only have to worry about specifying where we deviate from that. For this, I've opened a new draft PR #564 to try things out.

[fosterfarrell9]

You are certainly right. There are even more things where shopify deviates from Robocop's default settings that I do not like. We should not use the shopify settings as a basis. I am therefore closing this in favor of #564.