Deprecate --format disabled and add cop DisableCopComment

[jonas054]

The --format disabled option was implemented in #900. We (@fshowalter, @bbatsov, @yujinakayama and myself) had some discussion about it then, but I finally gave in and said that it was okay. I now think that I was wrong to do so, and that my initial comment about implementing the check as a cop was one that I should have fought harder for. Let me show you why.

Problems with the current implementation

The disabled formatter doesn't behave like a formatter. Instead of only controlling how the result is reported, it reports something different than the other formatters. If there are offenses, they are reported by all the other formatters, but not by -f d. The cops are still run, even if disabled is the only formatter given, but there's no reporting of their findings.

So the list of rubocop:disabled comments can only be reported in one format. We talked in #900 about the possible need to get the output in JSON format. I think I forgot to follow up on that concern in our discussion then, so that's not possible now.

There's something strange in the current reporting and its use of Infinity. If the file is not infinitely large, I think it's untrue to say that the range doesn't end. 😄 No-one has complained about this. Is that because the formatter isn't used much?

$ ./bin/rubocop -f d lib/rubocop/formatter/

Cops disabled line ranges:

lib/rubocop/formatter/base_formatter.rb:3..44: Metrics/LineLength
lib/rubocop/formatter/base_formatter.rb:3..Infinity: Lint/UnusedMethodArgument

Why a cop is better

Aside from avoiding the problems listed above, a cop solution where the cop is enabled by default also has the benefit of putting the offense count in .rubocop_todo.yml when --auto-gen-config is run.

The new code is smaller than the old. That's nice in itself, but I also think that it's cleaner to not send the disabled line ranges to all the fomatters in runner.rb. Not a big deal, but it caused a problem for me in another PR I'm working on.

What's missing

We don't get the reporting of line ranges with the cop. Admittedly that's a deterioration compared to the current functionality. [Edit: This is no longer the case. See further down.]

[bbatsov]

No-one has complained about this. Is that because the formatter isn't used much?

Guess so. Perhaps we didn't promote it enough. :-)

[fshowalter]

FWIW, I don't think a formatter OR a cop is the answer. I'd like to see the
disabled cops (and line ranges) available to any formatter as meta
information about the run. Like how RSpec will report how many tests were
skipped during a run., for example.

The original use case for the disabled formatter was for CI builds to give
you an at-a-glance look at the code's known cop exclusions, but we've since
extended it to code reviews.

If this were to go in as a cop, we'd have to tweak our builds to fail on
any violation except the skipped cops. But since they'd output like any
other violation, it would be harder to get the at-a-glance info, whereas
with something like RSpec, this kind of information is clearly separate.

Just some thoughts.

On Sat, Nov 1, 2014 at 7:20 AM Bozhidar Batsov notifications@github.com
wrote:

No-one has complained about this. Is that because the formatter isn't used
much?

Guess so. Perhaps we didn't promote it enough. :-)

—
Reply to this email directly or view it on GitHub
#1413 (comment).

[jonas054]

@fshowalter How about this? (Added two commits.)

Now you see the ranges, very similar to the output of the disabled fomatter today. Run with --only Style/DisableCopComment --format emacs for example. Or simple.

If this were to go in as a cop, we'd have to tweak our builds to fail on any violation except the skipped cops.

I don' think so. I chose to make the cop enabled by default so that it's included in .rubocop_todo.yml. I expect that every project that has rubocop:disable comments will disable the Style/DisableCopComment cop in configuration. But when you run with --only Style/DisableCopComment, the cop will run even if it's disabled in configuration.

[jonas054]

And about the presentation of disabled lines being too similar to other output. It's easy to make it stand out now:

[bbatsov]

I'm OK with the suggested change, although I'm not sure this cop should be enabled by default. Some users will definitely be surprised that disabling cops via comments yields offenses...

[jonas054]

Yes, I weighed the pros and cons of enabling by default and came out in favor of enabling, but I agree that it might be a source of irritation. If the cop is enabled when rubocop --auto-gen-config is run, there will be an entry in .rubocop.yml with an offense count. I thought this was a nice feature to have, but people who want that can enable the cop in their .rubocop.yml before the inherit_from: .rubocop_todo.yml. So, disabled by default it is then.

[yujinakayama]

Personally I don't think the problems listed above are compelling enough to replace the formatter with the new cop.

The disabled formatter doesn't behave like a formatter. Instead of only controlling how the result is reported, it reports something different than the other formatters.

I don't think all the formatters must report offenses. The multiple formatter feature allows users to combine them flexibly and that's why the feature exists.

So the list of rubocop:disabled comments can only be reported in one format. We talked in #900 about the possible need to get the output in JSON format. I think I forgot to follow up on that concern in our discussion then, so that's not possible now.

It's possible by creating a custom formatter using the second parameter of file_started method (though we haven't officially documented the cop_disabled_line_ranges key as a public API).

There's something strange in the current reporting and its use of Infinity. If the file is not infinitely large, I think it's untrue to say that the range doesn't end. 😄 No-one has complained about this. Is that because the formatter isn't used much?

I agree the wording Infinity (actually it's automatically printed by Ruby) is not smart enough, but I don't think replacing it with the actual last line number in the file improves understandability since the Infinity (or something like that) can indicates the difference between the comment-only-line and the end-of-line comment.

The new code is smaller than the old. That's nice in itself, but I also think that it's cleaner to not send the disabled line ranges to all the fomatters in runner.rb. Not a big deal, but it caused a problem for me in another PR I'm working on.

What's the problem?

[jonas054]

I don't think all the formatters must report offenses. The multiple formatter feature allows users to combine them flexibly and that's why the feature exists.

That's good, and maybe I didn't fully grasp the intention of multiple formatters, but when a concept such as disabled comments fits into the offenses category, it's easier IMO to let all formatters report offenses for now.

It's possible by creating a custom formatter using the second parameter of file_started method (though we haven't officially documented the cop_disabled_line_ranges key as a public API).

That sounds like work, as opposed to getting it for free like in my proposal.

I agree the wording Infinity (actually it's automatically printed by Ruby) is not smart enough, ...

I see now that it's difficult to show all the information you want in a good way. With the Infinity marker, you don't see how many lines are disabled, but you do see that it's the rest of the file. I'm willing to keep it as it was.

What's the problem?

I was experimenting with caching offenses to get faster runs. (Whether this will become a PR or just a ditched experiment remains to be seen.) Then I found that I would have to cache the disabled lines too, and thought that it would be easier to regard them as offenses. The changes I needed to make in order to simplify Runner#process_file turned out to be a simplification as well (IMO), so I thought it was a win-win.

[fshowalter]

Agree on both points.

On Sun, Nov 2, 2014 at 2:39 AM, Bozhidar Batsov notifications@github.com
wrote:

I'm OK with the suggested change, although I'm not sure this cop should be
enabled by default. Some users will definitely be surprised that disabling
cops via comments yields offenses...

—
Reply to this email directly or view it on GitHub
#1413 (comment).

[jonas054]

I was hoping for unanimous acceptance, but since I haven't been able to persuade @yujinakayama, I'll close this PR.