Rubocop: Improving Rubocop's defaults?

[ivoanjo]

Hello there!

Today I got to thinking a bit about Rubocop's defaults, and would like to solicit some feedback on the matter. I know that code styles are an easy way to create a flamewar, and I absolutely do not want to do so 😁
So please let's keep this to an awesome, respectful discussion!

So first and for all: BIG THANKS for Rubocop 👍👍👍

Having a consistent style used throughout a community and project are really useful things, prompting such tools as gofmt and elixir even including its own formatter as part of the official distribution. I fully agree with Sandi Metz's recent blog post on the matter.

Inside the Ruby community, Rubocop is the gold standard for code style checks, without question.
So whatever rubocop implements, is what the community is very likely to use.

Rubocop itself attempts to follow the community Ruby Style Guide.

So my issue is: It's my personal opinion that Rubocop's defaults are not up to par with what programmers are used to from other tools and language ecosystems. I'm not sure if that is because of limitations on the tool (e.g. it gets false positives when applying a rule that the community agrees with) or if the defaults on the community style guide need to be improved too.

My opinion also comes from the observation that I don't know of any project that doesn't ship with tweaks to the default Rubocop configuration.

For instance, I've collected the 5 all time most downloaded gems that use rubocop based off of rubygem's stats, and they all tweak the rubocop defaults in one way or another:

• bundler: [1], [2]
• rake: [1]
• activesupport/rails: [1]
• thor: [1]
• rspec: [1]

Even rubocop ships with its own tweaks to the defaults: [1], [2].

I think that Rubocop's excellent configurability and the potential for conflicts in such a discussion resulted on our community not having a serious discussion about a default, and everyone just agreeing to disagree, and going off on their own adding their own configuration.

I think this is not a great state to be in: consider a newcomer to the community. We tell them that Rubocop is the thing to follow, to learn from, if you want to write correct, high-quality Ruby. And then when Rubocop gives them confusing advice, we just teach them how to update the exceptions. And we do the same ourselves.

I would like to be a part of the solution, rather than the problem: So along with opening up the discussion on this, I would like to propose that we pick a number of gems that we believe are representative of what the ruby community should strive for.

We would then to then contact their authors, and identify if their code should be changed or if rubocop's defaults should be changed in order for them all to pass rubocop's checks with no warnings or issues without any tweaks to the defaults, and then start submitting PRs to get us toward that goal.
I think rubocop should be on that list, but beyond that we may want to pick from those gems I listed above, or to select others. Of course, I volunteer to do my best as part of all these efforts.

So here's my challenge: Can we improve Rubocop's defaults so that projects that the community agrees are high-quality can pass without needing exceptions? And thus by extension that new projects can be confident in just following Rubocop's recommendations without exception?

Thanks for reading thus far. I hope something interesting comes out of all of this 😃

[akhramov]

Can't agree more.

Just one note regarding bundler: bundler still supports Ruby 1.8, and therefore its Rubocop config prevents developers from using new shiny features we all like.

Also there is an ongoing discussion on Metrics/MethodLength and Metrics/AbcSize threshold relaxation. Please don't hesitate visiting #4588.

Thank you very much for raising this issue!

[ivoanjo]

You raise an interesting point: bundler is probably not helpful as an example as they strive to always support older versions of Ruby. (They are planning to drop < 2.0 in bundler 2.0, but 2.0 is still a bit too old).

It's probably not worth going all the way back to very old Ruby versions to change their defaults (at least at first), so yeah that's an important point to keep in mind.

Rubocop supports and requires Ruby >= 2.1 so I propose we start from Ruby 2.1+ as I believe that is a good trade-off.

[bbatsov]

Also there is an ongoing discussion on Metrics/MethodLength and Metrics/AbcSize threshold relaxation. Please don't hesitate visiting #4588.

While those are always going to be violated, the real question is what makes sense most of the time, but doesn't allow unnecessary complex code to creep in unnoticed. I think that fundamentally many people don't understand the point of some the cops (therefore our ongoing effort to improve the RuboCop manual). I've always been more fond of local exclusions of some checks as this makes it explicit that you understand something is complex, but you're fine with it. Updating defaults is a rather blunt way to address this problem which basically allows the complexity to grow across your entire app.

I'm not saying all of the defaults are great - there's always some room for analysis and improvement. We should just be smart about all of this.

[ivoanjo]

While those are always going to be violated, the real question is what makes sense most of the time, but doesn't allow unnecessary complex code to creep in unnoticed.

That's a valid point. But what is "makes sense most of the time" for a new Rubyist (or new to a project)?

My concern is that by not having a stricter set of defaults, we immediately seed confusion into a new user (or one that is new on a codebase): What do we find acceptable on this codebase? Is this cop ok to skip, or do I have to change my code to avoid the warning? To someone coming in as an outsider (for instance on an open-source project), these can be though questions.

Lots of open-source projects do employ Rubocop as part of their CI which is why I believe this is a big concern.

Furthermore, consider even programmers that are coming in from college, or even from other, stricter languages. They are used to following some code style tool and be done with it, and suddenly they find one that is much stricter and harder to satisfy. I believe it's not clear at all that they have to kind-of form an opinion about every Rubocop warning that is breaking their build.

Or consider a team that employs programmers using multiple programming languages. It's weird that if you're going through a code review and someone asks you about the N exceptions you've just added that you have to explain that Rubocop should basically taken "with a grain of salt". It's rather unexpected from other parties (at least that's my exact experience).

I think that fundamentally many people don't understand the point of some the cops (therefore our ongoing effort to improve the Rubocop manual).

I agree that this is a big part of the problem, especially because many cops just tell you "this is wrong" without: a) context of where they are useful and how they authors intended them to be considered when applied to a codebase, and b) how you would go about fixing it.

But for the reasons above I believe that even by improving the documentation, the original problem I presented above does not go away.

---

TL;DR: I believe that by not having a good set of defaults, we are putting off new Rubyists (with or without previous programming experience) from learning a lot of good practices (at least without someone experienced to provide the guidance on how they should consider or disregard Rubocop).

I also believe that we're making it hard for outside contributors to an open-source project to get their contribution in an acceptable format, as it can be completely unclear to them which cops should be skipped or that any should be skipped at all.

More than wanting to push some specific style I may enjoy writing Ruby in it, my main objective here is to embrace new Rubyists and new Rubocop users so they can make the most out of this amazing tool. But to change the status quo, I need the support from the Rubocop maintainers (someone's going to have to approve my PRs :stuck_out_tongue:), so this is why I am trying to sell you on my idea 😀

[bbatsov]

There hasn't been much activity on this ticket and our Core Team is spread too thin on so many tasks, so I'll just close it for the sake of having a cleaner lists of tasks to focus on. We'd gladly review a PR (or many PRs for that matter given the scope of all of this), but it's unlikely that'd we're going to tackle this ourselves in the foreseeable future.

Generally it'd be best of each default was reviewed on a case by case basis.