How to reduce risk when introducing new cops

[andyw8]

It seems that the addition of MigrationClassName resulted in a lot of issues. I think we should take some time to consider what process changes might prevent similar problems in future.

[andyw8]

(cc @koic, @pirj - not sure if GitHub sends notifications for new Discussions).

[pirj]

I always ask contributors to check their new cops against real-world-rails (or real-world-rspec for rubocop-rspec). This is not perfect, as some cops introduce thousands of new offences. Still, this is much better than nothing. There are certain repos, e.g. GitLab, with inventive coding, and reveal numerous problems with newly introduced cops.

I suggest introducing this practice to the PULL_REQUEST_TEMPLATE. Please notice that in rubocop-rails there's a single checklist for any kind of changes, for both updates to existing cops and for new cops. rubocop-rspec's template for contrast, has three distinct sections:

Before submitting the PR make sure the following are checked:
 - [ ] ...

If you have created a new cop:
 - [ ] ...

If you have modified an existing cop's configuration options:
 - [ ] ...

To sum it up, I suggest:

1. For rubocop-rails:

• split the PR template into three sections
• add "The cop is checked against real-world-rails repositories" to the "created a new cop" section

2. Add "The cop is checked against real-world-rspec repositories" to the "created a new cop" section in rubocop-rspec
3. Provide clear guidance on how to check against real-world code in those repos. It is not trivial due to some repos using stale RuboCop versions with removed cops, an when checking in bulk, rubocop exits with an error code. I usually remove all .rubocop*.yml files in those repos. This is not very convenient, but it works. Appreciate if you know of a better option. An inconclusive discussion on this topic.
4. Provide a clear guidance of what we expect from such a check:

• no cop errors
• no infinite loops/performance dips
• cop only flags legitimate offences
• provide information if the cop is safe/unsafe for inspection and autocorrection

[pirj]

cc @bquorning @Darhazer

[koic]

Yeah, tweaking the pull request template for new cops looks like a good idea.

[andyw8]

It is not trivial due to some repos using stale RuboCop versions with removed cops, an when checking in bulk, rubocop exits with an error code. I usually remove all .rubocop*.yml files in those repos. This is not very convenient, but it works. Appreciate if you know of a better option.

This would take some effort to setup, but perhaps we could create a Docker image containing real-world-rails already setup, and the problematic .rubocop*.yml files removed. That could run as a GitHub Action, as part of CI.

[andyw8]

Thanks for the suggestions, WIP PR: https://github.com/rubocop/rubocop-rails/pull/677/files