config/default: Upstream cop disables since this is the central place

[issyl0]

• In github/github when we bumped this gem and bumped the version of RuboCop and turned off DisabledByDefault, there were some cops that RuboCop had detected offenses for that weren't declared in here as explicitly disabled.
• Let's set Enabled: false for the new cops, so that this gem's config is the source of truth and downstream consumers don't diverge too much.
• I haven't made any effort to enable these new cops. I did search the styleguide, but I didn't see any rules specific to these new cops, so I'll leave that exercise for another time.

[issyl0]

Error: unrecognized cop or department Lint/RequireRangeParentheses found in config/default.yml
Did you mean `Lint/RequireParentheses`?
unrecognized cop or department Style/EmptyHeredoc found in config/default.yml
Did you mean `Style/EmptyMethod`?
unrecognized cop or department Style/MagicCommentFormat found in config/default.yml
Did you mean `Style/Documentation`?

Ah, we're using an older version of RuboCop in this project itself, where the tests run: https://github.com/github/rubocop-github/blob/main/Gemfile.lock#L52.

And the gem itself only specifies >= 1.0.0 so the "unknown cops" error will affect projects that use this gem that aren't on the latest and greatest RuboCop version.

Do we want to do this at all, in this case? Who makes the decisions about bumping the RuboCop version requirements?

[bensheldon]

I think, technically, we have to lock the Rubocop versions in this project, otherwise they will get out of sync with the consumers of this gem, and that would be a bad experience.

I don't like the idea of locking Rubocop versions via this project, but I think it's necessary.

There may be disagreement on this point though: I don't think that when we update, we should continually disable newly introduced gems. Instead, when a consumer updates this gem in their project, they become responsible for parsing through the changes and choosing what they want to adopt or reject.

[bensheldon]

Idea: We can set up Dependabot updates to propose updates on a weekly/monthly calendar.

[issyl0]

There may be disagreement on this point though: I don't think that when we update, we should continually disable newly introduced gems. Instead, when a consumer updates this gem in their project, they become responsible for parsing through the changes and choosing what they want to adopt or reject.

Indeed, I disagree. 😄 If we don't aim to get this to a source of truth for GitHub's Ruby projects, then we end up having to repeat this whole engineering initiative exercise several years down the line at best.

The experience for users of this gem within GitHub Ruby apps will be better now that we've got rid of DisabledByDefault - at least they'll be able to tell what's going on when they update this gem and see new cops (configured or not). And I suppose they can override any of our cops Enabled/Disabled settings with the one appropriate to their project's style.

I can see the reasons for not wanting to be too prescriptive here, but I'm worried that if we don't keep on top of versions and have a top-down stance of "everything here adheres to the styleguide", then there won't be the driver for individual projects to keep their RuboCop configurations in sync with the styleguide (and, indeed, feel empowered to update the styleguide if there's something new or an existing rule they strongly want to change)? Causing more divergence and confusion, surely?

[issyl0]

Worth noting I suppose that "everything here adheres to the styleguide" is not true at the moment for some of the configuration here, though we've made pretty good strides towards it. There are various TODOs for, just as an example, upstreaming configuration in github/github that does adhere to the styleguide.

[bensheldon]

Does this sound right?

• We should strict dependency lock in rubocop-github.
• When rubocop-github's strict dependencies are updated, the maintainers of this repo should (in sync) take an affirmative stance on whether to enable/disable newly introduced cops.
• Downstream consumers of rubocop-github, when they update rubocop-github then take their stance if they want to change their own project's configuration.

[issyl0]

@bensheldon Perfect!

[composerinteralia]

We used to have stricter constraints, and I removed them in #93 because they made it more difficult to upgrade rubocop in github/github. This library didn't have clear ownership at the time, and github/github ended up stuck on old versions of the rubocop gems.

Those were <= constraints, which both made it more difficult to upgrade and failed to coordinate the rubocop versions and configuration.

The ~> in this PR is better, since it does still allow some room for downstream projects to upgrade rubocop even if this library falls a bit behind. But if we fall too far behind it'll effectively become <= again.

I'd personally prefer a >= constraint. If we keep on top of things, we can bump the number whenever a new version of rubocop comes out. But if we ever fall behind, or ownership of this project fades away, folks downstream won't get stuck on an old version.

[bensheldon]

@composerinteralia thanks for the the feedback about ~> vs >= constraints. That sounds right to me 👍🏻