Remove DisabledByDefault

[robthornton]

With Rubocop now using semantic versioning, and thereby setting a clear boundary when pending cops become enabled by default, it doesn't make sense to keep this configuration option.

See: https://docs.rubocop.org/rubocop/versioning.html

Users of this gem will now get warnings about all previously disabled by default cops. They will either need to generate TODO lists using Rubocop, fix the violations, or manually configure rules they wish to ignore.

Consumers of the gem should also be aware of the NewCops configuration to enable or disable new, pending cops by default and thereby somewhat retaining the old behaviour.

[andyw8]

Should this be a 2.0 release?

[sambostock]

If we're removing this, we probably want to switch to explicitly disabling all existing rules that we haven't already got an entry for.

That way, we're only changing the behaviour going forward, instead of mass enabling rules on consumer repos.

Basically what you've done for Gemspec/RequiredRubyVersion, but we need to cover all the other rules which we might not detect this repo violating. Not sure if there's an easy way to list out all those rules though.

[robthornton]

This feels overly paranoid to me. It makes more sense to bump the major version (2.0) and say “breaking change, watch out” with tips on how to deal with it. I don't think blanket trying to hide 10's or 100's of Cops is the answer and defeats the purpose of this change.

It should be up to each project which cops to ignore or fix, just as what was done in this PR.

[andyw8]

Can we test this out on a substantial project before release, to better understand what the upgrade process will involve?

And also maybe add an upgrade guide in the wiki here?

[rafaelfranca]

Users of this gem should not need to make any decisions about cops. This gem is a style guide which means that all apps should be using the same set of rules, not making decisions witch ones enable or disabled. This means that this gems needs to disable all cops that we want disabled and enable all cops we want enabled. Just removing this config is not enough.

Also please don’t change the string quotes.

[robthornton]

As a first step, I've reverted the quotation changes and disabled that cop. There is an ongoing discussion (which I believe @sambostock will be sharing details on) about next steps. On reflection, I agree with your statement @rafaelfranca and it shouldn't be the responsibility of downstream projects (namely core) to decide which cops to enable/disable.

This change alone is not adequate.

[sambostock]

If I'm interpreting it correctly, it sounds like we're all agreed on the following problem

Because of DisabledByDefault: true, we've generally ignored (potentially useful) newly released cops

and the following improvement

We should explicitly enable or disable all existing cops

It sounds like what needs discussion is how we proceed with respect to new cops:

Alternative: Remove DisabledByDefault: true, and triage on major versions

This would mean we go with the community defaults for new cops going forward.

As @robthornton mentioned, as per Rubocop's Versioning Docs for 1.0 onwards, new cops being enabled will be accompanied by a major version update. We could limit the version of rubocop we support in this gem, and triage new cops on every major version update.

Effect on Rubocop extension gems

This would also have the side benefit of not applying DisabledByDefault: true to cops consumers add via gems (e.g. rubocop-performance).

As a side note, it may make sense to introduce rubocop-shopify-performance, rubocop-shopify-rails, etc. to provide a similar list of cops from the rubocop-performance, rubocop-rails, etc. gems which we agree with.

Alternative: Keep DisabledByDefault: true, and triage asynchronously

This would mean we keep our opinionated config for new cops going forward.

As Rubocop introduces new cops, we continue to disable them by default. We could triage these new cops whenever suits us:

• as needed/surfaced (e.g. someone suggests we enable some new cop)
• semi-automatically (e.g. open PRs to automatically add them to the config as they are released)
• manually on milestones (e.g. manually triage on major versions)

Unless we add some sort of automation, it would likely mean we continue to ignore new cops.

Effect on Rubocop extension gems

This would mean we would continue to disable all gem cops by default, which we should ensure we document somewhere. For example:

If you add rubocop-performance to your repo, note that its cops are disabled by default. You will need to add them individually to your .rubocop.yml.

[robthornton]

I thought about this problem a lot more over the holidays and I'm not sure this PR is the right thing to do at all. Rather, I think the problem might lie with documentation around being transparent about what this gem does (disabling all cops by default) and explaining how to add extensions like rubocop-rails or rubocop-performance. We should communicate that you must be explicit about which cops you want to enable, rather than just blinding turning them on by default.

I concur with the purpose of this gem, which is for Shopify as an organization to be opinionated about the style of Ruby we write. We should maybe be equally opinionated about adding additional extensions.

So, provided there are no major objections to this approach, I'll draft up a documentation change to reflect this. I think this should probably lie in the gem's documentation but perhaps there should be a note in the main style guide too?

[volmer]

I agree with all the points from @rafaelfranca, but I also understand the pain that users of multiple configs might be experiencing since AllCops/DisabledByDefault is a global setting that affects all extensions. I worked on this branch by changing the approach slightly. Now, in addition do removing DisabledByDefault, our rubocop.yml actually disables all unwanted cops, without affecting our overall config or introducing breaking changes. You can see in test/fixtures/full_config.yml for the effects of this change.