Consider removing DisabledByDefault: true from inherit_gem configuration

[bensheldon]

When maintaining a project Rubocop file, it becomes troublesome when an inherit_gem: configuration contains DisabledByDefault: true because it applies to the entire project's Rubocop configuration and is difficult to override.

We struggled with this when updating our github-rubocop rules, which are intended to behave in a similar inherit_gem: configuration. To summarize:

• The DisabledByDefault: true changes all rules in a project to be "opt-in rather than opt-out", and requires explicitly enabling any new rule as other Rubocop gems are updated.
• This global behavior change isn't obvious when it's contained in a Rubocop ruleset included via inherit_gem:.
• Developers will write new, custom cops in their own projects, with no configuration in .rubocop.yml, and those custom cops never run because all cops are disabled unless explicitly enabled.

The ideal configuration we arrived at is:

• Do not add DisabledByDefault: true to the base set of rules (e.g. this repo).
• Opinionatedly disable rules that we do not want.
• Encourage/document applications to add DisabledByDefault: true to their own rubocop.yml if they want all their rules to be explicitly opt-in.

[koic]

Firstly, rubocop-rails-omakase considers building rules from scratch if they don't align with specific use cases:
https://github.com/rails/rubocop-rails-omakase#contributing

Furthermore, using DisabledByDefault: true seems to impact the maintainability of rubocop-rails-omakase. If individual cops are enabled or disabled without utilizing DisabledByDefault: true, there will be a need to explicitly disable new cops introduced by RuboCop at some point. For more details on this, please refer to the policy on pending cops:
https://docs.rubocop.org/rubocop/1.59/versioning.html#pending-cops

This suggests that use cases unable to continue with DisabledByDefault: true might already be beyond the support scope of rubocop-rails-omakase.

As a side note, Jeremy Evans' "Polished Ruby Programming" also presents an approach using DisabledByDefault: true. Therefore, I think rubocop-rails-omakase should inherently include the DisabledByDefault: true setting in its "omakase" approach.

[bensheldon]

I appreciate that feedback! Here's what "Polished Ruby Programming" says about it:

RuboCop implements many checks, called cops, and most of the cops are enabled by default, even those not related to syntax. It can be tempting to use the RuboCop defaults, since it is otherwise daunting to go through every cop enabled by default and decide whether you want to enforce it. Thankfully, RuboCop has a solution for this, which is the configuration parameter, AllCops:DisabledByDefault. Using this configuration parameter, you can only enable the syntax checks that you believe will be helpful for your library.

One approach to trying to satisfy the philosophers on the team without undue irritation to the poets is to start with all of RuboCop's cops disabled, except those related to syntax issues that have previously been complained about during code review. Then, as future code reviews happen, if one of the philosophers complains about a new syntax issue that is available as a RuboCop cop, you can consider enabling that cop. Using this approach, you avoid many unnecessary syntax checks, and focus on only the syntax checks that your team finds beneficial.

The context I'll offer in response to that is that Rubocop frequently serves more purposes than solely debatable stylistic rules. In my experience, rules also exist for Security, Availability, and Performance reasons, which are "written in blood" so to speak. It was these in particular that caused us concern when we discovered that DisabledByDefault: true was inadvertently/surprisingly disabling them.

Another alternative, that hopefully wouldn't add too much complexity, would be to offer separate files for individual Cop rules (rules.yml maybe), as well as the more comprehensive AllCops configuration (rubocop.yml maybe), which would offer both an omakase default while also allowing reasonable flexibility in line with the readme:

Use them whole, use them as a starting point, use them as inspiration, or however you see fit.

[bensheldon]

The other learning I should describe from rubocop-gitHub was that we had 3 different needs that we needed to detangle:

• Recommended cop rules (enabled/disabled/options)
• Our own custom cop implementations (and their default yaml config), some of which are specific to our own compliance rules, but also some that we strongly like but haven't successfully moved upstream to more generic projects (rubocop, rubocop-rails, etc).
• Templates for new projects to generate their own rubocop.yml

Adding a global DisabledByDefault: true made it challenging/surprising for projects that wanted to adopt only a subset.

Ok, I think that describes all of my learnings 😊

[matthewd]

From the documentation linked above, it sounds like AllCops: NewCops: disable, combined with explicitly disabling all default cops, might give a better experience, at the expense of having to carry a list of all the default cops that existed as at 1.0. By my reading, that will give a stable warning-free state up until 2.0 (at which point the gem will need to refresh that explicit-disable list), without the awkwardness of surprise-disabling local / 3rd-party cops that applications have explicitly created/imported themselves.

(ISTM the ideal would be that Rubocop would offer a DisableBuiltinRulesByDefault option, providing a less laborious way for people to use it as an opinions-removed tool without having to choose between "break custom cops by default" and "list all built-in rules individually"... but absent that, at least the documentation claims the latter list is major-version stable.)

[bbatsov]

Well, you're more than welcome to request such changes on RuboCop's issue tracker. I'm often surprised in how different teams are using RuboCop, so it's good to keep track of their use-cases in a central place.

Adding a global DisabledByDefault: true made it challenging/surprising for projects that wanted to adopt only a subset.

Perhaps we can make the current setting more flexible - e.g. other than the boolean values it can have something like a list of departments to disable explicitly, which I think would be the most flexible option for everyone. (think ["style", "performance"]) Or rather do this the other way around - explicitly list the departments to enable, as that's usually more robust.

[jeremy]

Furthermore, using DisabledByDefault: true seems to impact the maintainability of rubocop-rails-omakase. If individual cops are enabled or disabled without utilizing DisabledByDefault: true, there will be a need to explicitly disable new cops introduced by RuboCop at some point.

Precisely this.

From the documentation linked above, it sounds like AllCops: NewCops: disable, combined with explicitly disabling all default cops, might give a better experience, at the expense of having to carry a list of all the default cops that existed as at 1.0.

Worth exploring this route in a PR if anyone's game.