Make pending cops warning slightly more useful

[colszowka]

As proposed earlier in #8413 this changes the default pending cops warning message to be copy&pasteable.

While writing the specs I also learned about NewCops: enable, which probably is what I was originally looking for, so I decied to add that to the warning as well so users are aware of their options.

Output now looks like this:

The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file.

Please also note that can also opt-in to new cops by default by adding this to your config:
  AllCops:
    NewCops: enable

Lint/DuplicateRescueException: # (new in 0.89)
  Enabled: true
Lint/MissingSuper: # (new in 0.89)
  Enabled: true
Lint/MixedRegexpCaptureTypes: # (new in 0.85)
  Enabled: true

Output before this change:

The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file:
 - Lint/DuplicateElsifCondition (0.88)
 - Lint/MixedRegexpCaptureTypes (0.85)
 - Style/AccessorGrouping (0.87)

---

Before submitting the PR make sure the following are checked:

• 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.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[koic]

Thank you for opening the PR. However, this proposal has not been accepted in the past.
#7765 (comment)

@bbatsov What do you think about that?

[marcandre]

"new in N/A" sound quite odd, maybe remove it altogether?

[colszowka]

Yeah, was thinking the same

[marcandre]

However, this proposal has not been accepted in the past.

Interesting. I wouldn't say it was "not accepted" though.

Let's make this warning more easily actionable...

I would put the NewCops at the end of the warning, add a URL to the cop when there is one (more so than which version it was added in, since what a cop does is important, when it was added isn't) and call it a day.

[bbatsov]

I'm slightly worried that we're going overboard with this message, as it already links to the documentation and that's probably enough. I definitely don't see much point in repeating Enabled: true for each cop in the message.

[marcandre]

Let's imagine a user that has read (and remembers) the documentation. That user presumably hasn't set NewCops to anything, wants to resolve the issue at hand (new cops, do I want them or not?), so the information needed is: what do these cops do, and a quick way to edit the conf:

# https://docs.rubocop.org/rubocop/cops_lint.html#lintduplicatecasecondition
Lint/DuplicateRescueException: 
  Enabled: true
# https://docs.rubocop.org/rubocop/cops_lint.html#lintmissingsuper
Lint/MissingSuper:
  Enabled: true
...

For a user that has not read the documentation, the bit onNewCops seems to me to be helpful too, but I'm less certain about its need.

Otherwise, how do you imagine a user resolving this? googling each cop in turn, then copy-pasting the name, not forgetting the : and typing Enabled: true or false?

Alternative would be a CLI command that would open the documentation link for each and prompt the user if the cop should be enabled, but seems a bit much.

[bbatsov]

Fair enough, you convinced me this might be useful. I like the format you proposed, but we also have to stuff somewhere the version when the cop was introduced (e.g. around the URL).

[marcandre]

Great 😸

So we could keep it where @colszowka is proposing:

# https://docs.rubocop.org/rubocop/cops_lint.html#lintduplicatecasecondition
Lint/DuplicateRescueException:  # (new in 0.89)
  Enabled: true
# https://docs.rubocop.org/rubocop/cops_lint.html#lintmissingsuper
Lint/MissingSuper:  # (new in 0.66)
  Enabled: true
...

[colszowka]

By the way, I saw that the cop metadata object also has a short description string, maybe it would be better to just put that one instead of the URL? Then no need to even open the link in a browser is neccessary to get what's going on.

@bbatsov

we're going overboard with this message, as it already links to the documentation and that's probably enough

Personally from first-hand experience I can say that:

• I have no problem with scrolling through the message if it ends up long. Usually on updates it's a few cops though so it's not that much more stuff
• Adding the cops to the config based on the current output was very annoying because you have to adjust each line and then add the enabled thingy. Having it copy&pasteable saves a minute here and there, hence the proposal
• Maybe I am too lazy / ignorant but based on the message currently printed AND me not knowing that the NewCops config option exists I did not get to the idea to go look at the docs. I actually checked the changelog once looking for such a thing because probably that's what I actually want to use, but could not find any mention of it. So that's why I suggest to put this on the warning output itself, in case users actually are looking basically for the "legacy behavior"

[marcandre]

Most of the descriptions seem an "english translation" of the cop's name. I say let's go with the URL and the copy-pasteable format and it should be a nice improvement on what we have now

[bbatsov]

Lint/DuplicateRescueException: # (new in 0.89)

I'm not big on inline comments in a hash, so I'd rather have this as:

# https://docs.rubocop.org/rubocop/cops_lint.html#lintduplicatecasecondition (added in 0.89)
Lint/DuplicateRescueException: 
  Enabled: true

# or

# https://docs.rubocop.org/rubocop/cops_lint.html#lintduplicatecasecondition
# Added in 0.89
Lint/DuplicateRescueException: 
  Enabled: true

We might also have to add the gem name there if we want to this to be even more informative, although it can easily the inferred from the URL.

[bbatsov]

Most of the descriptions seem an "english translation" of the cop's name.

Yeah, they certainly don't add much value. I've been pondering for years whether we should just delete them.

[bbatsov]

@colszowka Heads up - I'm planning to cut a new release soon, and it'd be nice if this PR made its way there.

[colszowka]

Sure thing, question is: Where do I get the URL for the cop from? I realized that this is not part of the cop struct or it's metadata passed to the method :/

{"Description"=>
  "Checks for places where string concatenation can be replaced with string interpolation.",
 "StyleGuide"=>"#string-interpolation",
 "Enabled"=>"pending",
 "Safe"=>false,
 "VersionAdded"=>"0.89"}

[marcandre]

Good point. Looks like you'll have to create a method in CopsDocumentationGenerator that will you this code for the URL and probably ask Yard for the fragment. Would you like some help on that?

[colszowka]

@marcandre Well if you have some related code snippet handy that might reduce effort on my end indeed :) However, the bigger question I have is: How does this work for plugins / cops extracted to i.e. rubocop-rails?

Not sure about the underlying code but I suspect it might be useful to actually change this at the root and always put the url into the metadata object wherever it is generated, what do you think?

[bbatsov]

It'd be nice to wrap this up before 1.0. If fetching the URL is problematic I guess you can just rebase, so we can merge this as is and improve it down the road.

[marcandre]

Oops, forgot about this. I'll create a PR adding documentation_url.

[marcandre]

Done in #8579

[bbatsov]

I'll merge this as is and we can add the doc url separately.

[colszowka]

Sorry for the lag, I was on vacation ⛰️ - Thanks for getting this merged!