[Fix #7077] Rename cops to meet the cop naming guideline

[pirj]

fixes #7077, with an exception, see below
fixes #10089 along the way

Not Done

We also need to put something in the docs on the subject and maybe introduce an internal affairs cop that checks for some naming anti-patterns.

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• [-] Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• [-] Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[bbatsov]

Might also be a good idea to spell out the naming guidelines explicitly in the docs and add some internal affairs cop that looks for common problems (mostly problematic words appearing in the name or in the wrong order). I'm also wondering if we can come up with a good replacement for things like PercentArray, as I hate this terminology - after all those are just different array/string/etc literals.

Perhaps we should be spelling out Def fully as Definition. We have to see what has been the predominant practice so far.

[pirj]

spell out the naming guidelines explicitly

I've added a small note to to the Development section of our docs, as it seems to be the only one on the topic of creating new cops:

Name the cop to explain the offense it detects. See the https://github.com/rubocop/rubocop/blob/master/config/obsoletion.yml:["renamed" section of `config/obsoletion.yml] for good and bad examples (old name is on the left, new name on the right).

After a while, my old idea of writing it as:

Name the cop to explain the offense it detects, not where or what the subject of the offense is.

does not seem very specific and guiding. There are some good practices behind good cop naming, but I miserably fail to extract them into concise statements.

[bbatsov]

You don't have to be super concise. ;-)

[pirj]

Docs (with reference formatting fixes) for easier view https://github.com/rubocop/rubocop/blob/28d52cb0d2c730c7f854961170c8480c85529973/docs/modules/ROOT/pages/development.adoc#choose-a-name

[bbatsov]

Just to confirm - right now that's not a breaking change, right? I recall @dvandersluis worked on this a while back.

[bbatsov]

Why not make these bullet points?

[pirj]

right now that's not a breaking change, right? I recall @dvandersluis worked on this a while back.

It is unfortunately:

Error: The `Layout/IndentationConsistency` cop has been renamed to `Layout/InconsistentIndentation`.
(obsolete configuration found in .rubocop.yml, please update it)

[dvandersluis]

I added a way to set severity: warning for some types of obsoletions (which just outputs a warning instead of an error) but it is not set up for renames presently. I could add it if desired?

[dvandersluis]

Actually I remember now why we didn't. I tried implementing warnings for renames, but we still get an error because there's some other checks, for instance ConfigValidator raises its own error if there is configuration for a cop that does not exist, so we end up with something like this:

(yellow is the warning from ConfigObsoletion, red is the error from ConfigValidator)

and then RuboCop quits anyways. So there doesn't look like there's an easy way to make a warning happen here. Even if it didn't error, the cop class itself wouldn't know where to find it's config.

What we've done in some cases for renamed params is have the config handle both the old (deprecated) case and the new renamed case, and then update the cop to use both. We can't really easily do that with full cop renames though...

[dvandersluis]

I have an idea about how we might be able to solve this problem, I'm going to play with it tomorrow.

In the meantime, as @pirj said, this would be a breaking change.

[pirj]

@bbatsov Does this guide look complete?

[pirj]

@dvandersluis Any breakthrough on the idea to painlessly rename cops between major version updates?

[dvandersluis]

@pirj I’ve been away from home but I’m hoping to get back to it in next week.

[pirj]

@dvandersluis We came up with an idea to obsolete cops without actually removing them, see rubocop/rubocop-rspec#1519. The idea is to:

• rename the cop
• inherit the old cop from the new one
• keep the configuration in config/default.yml for both
• introduce an entry in config/obsoletion.yml

That way, there's an obsoletion warning, but no error trying to load the config.

I can rework this PR in the same way. Would you consider this a non-breaking, mergeable change?

In 2.0 we'll have to remove cops with the old names along with their configs and that's it.

[dvandersluis]

Hey @pirj, sorry that I obviously never had the time to work on this like I wanted to. I think your solution makes sense!

[bbatsov]

I agree that the new proposal is a non-breaking one (at least in my book) and provides a nice path to complete removal down the road.

[koic]

This looks like a good way. One thing, can we use assignment (NewNameCop = ObsoleteNameCop) instead of inheritance (class NewNameCop < ObsoleteNameCop) ?

[pirj]

can we use assignment?

That was the initial approach, I can't recall why I've switched to inheriting, I'll check if it works.

[pirj]

can we use assignment (NewNameCop = ObsoleteNameCop) instead of inheritance (class NewNameCop < ObsoleteNameCop) ?

Probably, but the doc generation task breaks.
Our goal is to keep the documentation for both until the major version update, right?
We have to tell YARD to parse a const definition as a class definition:

# Some cop docs
# !@parse class class NewNameCop < ::RuboCop::Cop::Base; end
NewNameCop = ObsoleteNameCop

But for some reason, documentation above the !@parse directive is not considered as the class doc. I'll investigate why.

It appears that this is the only way in any case, as obsoletion errors sometimes appear when inheritance is used.

[pirj]

Luckily, there's a workaround:

# !@parse
#   # Some cop docs
#   class NewNameCop < ::RuboCop::Cop::Base; end
NewNameCop = ObsoleteNameCop

See rubocop/rubocop-rspec#1519 for more details.

[koic]

As I left a comment on rubocop/rubocop-rspec#1519 (comment), the current implementation should have used inheritance (class NewNameCop < ObsoleteNameCop). I forgot to consider Base.inherited 💦 Please see link for details.

[pirj]

My apologies, @koic, I'm not convinced enough that duplicate output from the old and the new cop that is an impact on user experience is a good tradeoff for not modifying the code that builds cops' documentation.

I may miss something important, but the duplicate output was my experience with the inheritance approach.