add option to show cops disabed by inline comments #900

fshowalter · 2014-03-17T23:25:36Z

Sometimes its tempting for developers, especially juniors who may have trouble understanding some of RuboCop's warnings, to abuse the # rubocop:disable functionality. This pull request adds an option (-x or --show-disabled) that, after a run, prints a list of all the files, line ranges and corresponding cops that were ignored during the run due to # rubocop:disable comments.

jonas054 · 2014-03-18T09:31:41Z

You've added reporting outside of the formatter classes, and I see some problems with that. For example, you can' get this information in JSON format (--format json). Perhaps a better solution would be to implement this functionality in a new cop, disabled by default.

fshowalter · 2014-03-18T13:58:42Z

Yeah, I thought about adding formatter support, but was hesitant since those seem designed for offenses. I don't think this should go in as a cop, as it's not really an offense (and how would you disable it inline?).

Agree though, that this needs to be added to the JSON formatter (think it would make sense in the summary object) and I'll append with support for it tonight.

As for the rest of the formatters, they don't seem to support summary stats so maybe this info just isn't part of those formatters directly. In the current implementation it should still be printed to the output. (IE the fuubar formatter would do the nice progress bar =======> then afterwards it would print the ignored cops). Workable compromise?

jonas054 · 2014-03-18T14:49:45Z

Yeah, if that cop is disabled inline there's a risk that RuboCop implodes and erases itself from disk. I should have thought of that. 😉

I hope @bbatsov and @yujinakayama can offer their opinions on the matter. I still think a cop is a less intrusive solution.

bbatsov · 2014-03-18T19:10:49Z

I'm not overly fond of the cop idea, because this cop should be disabled by default and currently disabled cops can't be run with --only CopName (which I think is a bug). If we change the --only behavior I might be like the cop idea better. Also this would be a cop that will have generally have to be run by itself as I cannot imagine always running it (unless you're fanatically devoted to perfect style).

Generally I like more the flag idea. I'm not sure this info belongs inside the formatters, maybe printing it by itself is OK. Of course, we have to hear @yujinakayama's point of view.

yujinakayama · 2014-03-19T13:02:37Z

An optional idea: When -x/--show-disabled is specified, RuboCop only finds and outputs the # rubocop:disabled comments without running cops.

I think this would solve the issue about mixed outputs and also allows you to just check the disabled lines without waiting for long running cop inspection.

What do you think?

fshowalter · 2014-03-19T14:01:38Z

That could work, but I would imagine the most common use-case for this would be in a CI report in which case you'd run both of them together anyway. Since the JSON formatter is the only one really effected by this (since the JSON is presumably parsed so the extra text at the end wouldn't work) why don't I just add support for that formatter?

fshowalter · 2014-03-20T01:52:39Z

Okay, I moved the summary into the formatters. It does feel better in there, as just dumping the text at the end would potentially choke clients consuming the JSON formatter. That said, the formatters don't lend themselves to extension and I had alter the base formatter's signature like so:

# @api public
#
# Invoked after all files are inspected, or interrupted by user.
#
# @param inspected_files [Array(String)]
#   the inspected file paths.
#   This would be same as `target_files` passed to `#started`
#   unless RuboCop is interrupted by user.
# @param summary [Hash]
#   optional meta information about the completed run.
# @return [void]
def finished(inspected_files, summary = {})
end

Since this is a breaking change to the public API, maybe we can go with my previous commit and save this for v.19? That said, maybe consider adding a splat to the end of the base formatter methods to facilitate future extensions without breaking consumers? What's everyone think?

bbatsov · 2014-03-22T16:27:54Z

lib/rubocop/comment_config.rb

@@ -20,6 +20,11 @@ def cop_enabled_at_line?(cop, line_number)
      disabled_line_ranges.none? { |range| range.include?(line_number) }
    end

+    def disabled_line_ranges
+      disabled = @cop_disabled_line_ranges || {}
+      disabled.select { |key, value|  value.any? }


key -> _key

@yujinakayama It'd be nice if we checked for unused block arguments.

bbatsov · 2014-03-22T16:35:33Z

I'm not sure I like the addition of the summary param. Perhaps @jonas054 and @yujinakayama would suggest a better approach to handle this (assuming with go with adding the info to formatters).

It was said in rubocop#900 that it is a bug for --only to not enable the cop.

jonas054 · 2014-03-22T21:33:33Z

rubocop-todo.yml

+
+AllCops:
+  Excludes:
+    - vendor/**


Huh? What's this?

My bad, (I bundle locally so without it the spec's try and analyze every gem's files). Will fix.

jonas054 · 2014-03-22T22:29:39Z

I think this feature is a good idea, but I don't like how its addition requires so many changes in the core logic. A new cop is the way to go. We could do a little bit of hard-coding in comment_config.rb -- the only slightly ugly part of this solution -- to make sure this cop can't itself be disabled by comments. And we can still have a new option --show-disabled that runs the cop.

yujinakayama · 2014-03-23T13:53:54Z

but I don't like how its addition requires so many changes in the core logic.

I agree, the current implementation adds too much complexity for a feature.

Here's my idea I finally figured out:

Add new disabled_lines (not sure if the name should include _) formatter. This formatter reports the disabled lines at the final summary and can be used like the offenses formatter. (e.g. rubocop --format progress --format disabled_lines)
Pass the disabled line range hash to formatter's #file_started. As I mentioned about it in my initial formatter design, the second hash argument of #file_started is intended to be extended for like this case. So, we don't need to introduce formatter API change.
Just need to move the source parsing handling from Cop::Team#inspect_file to FileInspector#process_file (only the several lines) so that FileInspector can pass the disabled line hash to formatters.

bbatsov · 2014-03-23T16:12:44Z

👍 for @yujinakayama's idea

fshowalter · 2014-03-23T19:06:56Z

I like it. I'll make the changes tonight.

fshowalter · 2014-03-25T01:34:17Z

Ok, here's a first-pass at @yujinakayama's suggestion. What's everyone think?

yujinakayama · 2014-03-25T16:29:58Z

lib/rubocop/cop/team.rb

@@ -13,6 +13,7 @@ def initialize(cop_classes, config, options = nil)
        @config = config
        @options = options || { auto_correct: false, debug: false }
        @errors = []
+        @cops_disabled_in_comments = {}


This is unnecessary.

Agreed. Will fix tonight.

yujinakayama · 2014-03-25T17:06:27Z

lib/rubocop/options.rb

@@ -102,8 +103,7 @@ def add_flags_with_optional_args(opts)

    def add_boolean_flags(opts)
      option(opts, '-d', '--debug', 'Display debug info.')
-      option(opts,
-             '-D', '--display-cop-names',
+      option(opts, '-D', '--display-cop-names',


Please do not include unrelated changes to the commit.

Sorry, this was a holdover from the previous take which had an added option.

fshowalter · 2014-03-26T01:33:48Z

Think I got all of @yujinakayama's suggestions in there, but I'm sure a second set of eyes will prove otherwise. 😏

bbatsov · 2014-03-27T11:49:26Z

The build is broken.

fshowalter · 2014-03-27T12:56:00Z

Heh, jruby always seems to get me. I may need to do a push disabling the global rescue in cli.rb so Travis can show me the error, since I don't have jruby locally, but regardless, I'll get this resolved.

yujinakayama · 2014-03-27T17:43:07Z

lib/rubocop/options.rb

@@ -4,6 +4,7 @@

 module Rubocop
  # This class handles command line options.
+  # rubocop:disable ClassLength


Please increment the max class length in .rubocop-todo.yml rather than disabling.

yujinakayama · 2014-03-27T18:00:29Z

Looks good to me except the few comments and the build failure.

@bbatsov, @jonas054 Any other comments?

bbatsov · 2014-03-27T18:34:29Z

Nope, I have nothing to add to your comments.

jonas054 · 2014-03-27T20:15:47Z

rubocop-todo.yml

@@ -17,4 +17,4 @@ CyclomaticComplexity:
 # Offense count: 105
 # Configuration parameters: CountComments.
 MethodLength:
-  Max: 22
+  Max: 22


So you've removed the last newline in the file? No big deal, but it does seem a bit odd.

jonas054 · 2014-03-27T20:19:55Z

I only had one tiny remark. And I'm still moping over our choice of formatter rather than cop solution. 😄 No, that's alright.

fshowalter · 2014-03-28T01:15:39Z

Think that fixes the rbx/jruby errors, @yujinakayama's catches, and even adds back in the missing line for @jonas054. 😄

bbatsov · 2014-03-28T06:57:27Z

@fshowalter Please, rebase this on top of the current master and we'll finally be good to go.

fshowalter · 2014-03-28T23:40:01Z

Rebased. Last chance @jonas054 😉

jonas054 · 2014-03-29T06:56:59Z

No, I've had my chances. 😄 Godspeed, young Showalter!

…ments add option to show cops disabed by inline comments

bbatsov reviewed Mar 22, 2014
View reviewed changes

jonas054 added a commit to jonas054/rubocop that referenced this pull request Mar 22, 2014

Make --only enable the given cop

f32bd4a

It was said in rubocop#900 that it is a bug for --only to not enable the cop.

jonas054 mentioned this pull request Mar 22, 2014

Make --only enable the given cop #915

Merged

jonas054 reviewed Mar 22, 2014
View reviewed changes

bbatsov added the enhancement label Mar 23, 2014

jonas054 mentioned this pull request Mar 23, 2014

Fix default exclusion of vendor directories #917

Merged

yujinakayama reviewed Mar 25, 2014
View reviewed changes

yujinakayama reviewed Mar 27, 2014
View reviewed changes

jonas054 reviewed Mar 27, 2014
View reviewed changes

add formatter to show cops disabled in comments

6b1047e

bbatsov added a commit that referenced this pull request Mar 29, 2014

Merge pull request #900 from fshowalter/add_show_cops_disabled_by_com…

b828661

…ments add option to show cops disabed by inline comments

bbatsov merged commit b828661 into rubocop:master Mar 29, 2014

jonas054 mentioned this pull request Oct 31, 2014

Deprecate --format disabled and add cop DisableCopComment #1413

Closed

jonas054 mentioned this pull request Mar 29, 2020

implement Style/DisableCopsWithinSourceCodeDirective cop #7384

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to show cops disabed by inline comments #900

add option to show cops disabed by inline comments #900

fshowalter commented Mar 17, 2014

jonas054 commented Mar 18, 2014

fshowalter commented Mar 18, 2014

jonas054 commented Mar 18, 2014

bbatsov commented Mar 18, 2014

yujinakayama commented Mar 19, 2014

fshowalter commented Mar 19, 2014

fshowalter commented Mar 20, 2014

bbatsov Mar 22, 2014

bbatsov commented Mar 22, 2014

jonas054 Mar 22, 2014

fshowalter Mar 23, 2014

jonas054 commented Mar 22, 2014

yujinakayama commented Mar 23, 2014

bbatsov commented Mar 23, 2014

fshowalter commented Mar 23, 2014

fshowalter commented Mar 25, 2014

yujinakayama Mar 25, 2014

fshowalter Mar 25, 2014

yujinakayama Mar 25, 2014

fshowalter Mar 25, 2014

fshowalter commented Mar 26, 2014

bbatsov commented Mar 27, 2014

fshowalter commented Mar 27, 2014

yujinakayama Mar 27, 2014

yujinakayama commented Mar 27, 2014

bbatsov commented Mar 27, 2014

jonas054 Mar 27, 2014

jonas054 commented Mar 27, 2014

fshowalter commented Mar 28, 2014

bbatsov commented Mar 28, 2014

fshowalter commented Mar 28, 2014

jonas054 commented Mar 29, 2014

add option to show cops disabed by inline comments #900

add option to show cops disabed by inline comments #900

Conversation

fshowalter commented Mar 17, 2014

jonas054 commented Mar 18, 2014

fshowalter commented Mar 18, 2014

jonas054 commented Mar 18, 2014

bbatsov commented Mar 18, 2014

yujinakayama commented Mar 19, 2014

fshowalter commented Mar 19, 2014

fshowalter commented Mar 20, 2014

Choose a reason for hiding this comment

bbatsov commented Mar 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas054 commented Mar 22, 2014

yujinakayama commented Mar 23, 2014

bbatsov commented Mar 23, 2014

fshowalter commented Mar 23, 2014

fshowalter commented Mar 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fshowalter commented Mar 26, 2014

bbatsov commented Mar 27, 2014

fshowalter commented Mar 27, 2014

Choose a reason for hiding this comment

yujinakayama commented Mar 27, 2014

bbatsov commented Mar 27, 2014

Choose a reason for hiding this comment

jonas054 commented Mar 27, 2014

fshowalter commented Mar 28, 2014

bbatsov commented Mar 28, 2014

fshowalter commented Mar 28, 2014

jonas054 commented Mar 29, 2014