-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate --format disabled and add cop DisableCopComment #1413
Conversation
c2259c0
to
ad2e538
Compare
Guess so. Perhaps we didn't promote it enough. :-) |
FWIW, I don't think a formatter OR a cop is the answer. I'd like to see the The original use case for the disabled formatter was for CI builds to give If this were to go in as a cop, we'd have to tweak our builds to fail on Just some thoughts. On Sat, Nov 1, 2014 at 7:20 AM Bozhidar Batsov notifications@github.com
|
@fshowalter How about this? (Added two commits.) Now you see the ranges, very similar to the output of the
I don' think so. I chose to make the cop enabled by default so that it's included in |
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... |
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 |
There are a number of problems with implementing the search for inline disable comments through a formatter. A cop solves the same thing nicely. This deprecates the --format disabled option. The cop is disabled by default.
9b1323a
to
1716d4e
Compare
Personally I don't think the problems listed above are compelling enough to replace the formatter with the new cop.
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.
It's possible by creating a custom formatter using the second parameter of
I agree the wording
What's the problem? |
That's good, and maybe I didn't fully grasp the intention of multiple formatters, but when a concept such as
That sounds like work, as opposed to getting it for free like in my proposal.
I see now that it's difficult to show all the information you want in a good way. With the
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 |
Agree on both points. On Sun, Nov 2, 2014 at 2:39 AM, Bozhidar Batsov notifications@github.com
|
I was hoping for unanimous acceptance, but since I haven't been able to persuade @yujinakayama, I'll close this PR. |
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 ifdisabled
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?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.]