Add Todo alias and make --disable-autocorrect mark Cops as Todo #7335

desheikh · 2019-09-08T05:00:50Z

We want to to differentiate between disables made intentionally due to false positives, or that we don't plan on fixing vs disables generated using the --disable-uncorrectable flag, which ideally should be reviewed at some point.

#4355 discussed adding a todo directive as part of the disable-uncorrectable however it was never merged.

This is a slightly different approach which adds "todo" as a full alias to "disable" instead and either can be used interchangeably.
The --disable-uncorrectable flag will now generate comments using the todo directive instead of disable.

example:

# rubocop:todo Metrics/AbcSize
def my_method
...
end
# rubocop:enable Metrics/AbcSize

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.

desheikh · 2019-09-10T14:26:16Z

@jonas054 as you added in the disable-uncommentable flag, do you have any feedback on this?

jonas054

This is exactly what I had in mind. Or slightly better than that, actually. I like the [Todo] output. I just had a couple of comments on the test code.

jonas054 · 2019-09-10T18:48:51Z

spec/rubocop/cop/style/double_cop_disable_directive_spec.rb

@@ -3,7 +3,7 @@
 RSpec.describe RuboCop::Cop::Style::DoubleCopDisableDirective do
  subject(:cop) { described_class.new }

-  it 'registers an offense when using `#bad_method`' do
+  it 'registers an offense when using `#bad_method` with disable' do


It seems I was the one who named this example originally, but now it makes no sense to me. I don't know what #bad_method or #good_methodmeans. 🙂 Could you come up with something better?

Addressed :)

jonas054 · 2019-09-10T18:49:42Z

spec/rubocop/cop/style/double_cop_disable_directive_spec.rb

@@ -15,6 +15,18 @@ def choose_move(who_to_move) # rubocop:disable Metrics/CyclomaticComplexity
    RUBY
  end

+  it 'registers an offense when using `#bad_method` with todo' do
+    expect_offense(<<~RUBY)
+      def choose_move(who_to_move) # rubocop:todo Metrics/CyclomaticComplexity # rubocop:todo Metrics/AbcSize # rubocop:todo Metrics/MethodLength


How do we handle mixed rubocop:disable and rubocop:todo? Another example?

Based on the current implementation of the cop we're essentially stripping down to just the first cop.

jonas054 · 2019-09-11T04:37:00Z

CHANGELOG.md

@@ -59,6 +59,7 @@
 * [#7215](https://github.com/rubocop-hq/rubocop/issues/7215): Make it clear what's wrong in the message from `Style/GuardClause`. ([@jonas054][])
 * [#7245](https://github.com/rubocop-hq/rubocop/pull/7245): Make cops detect string interpolations in more contexts: inside of backticks, regular expressions, and symbols. ([@buehmann][])
 * Deprecate the `SafeMode` option. Users will need to upgrade `rubocop-performance` to 1.15.0+ before the `SafeMode` module is removed. ([@rrosenblum][])
+* [#7335](https://github.com/rubocop-hq/rubocop/pull/7335): Add todo as an alias to disable. `--disable-uncorrectable` will not disable Cops using `rubocop:todo` instead of `rubocop:disable`. ([@desheikh][])


not -> now
and also Cops -> cops (I see no reason to capitalize it)

jonas054 · 2019-09-11T05:29:45Z

I found something in the changelog, so please take a look. Could you also search through all the .md files under manual for rubocop:disable and see if you find more places where we should mention rubocop:todo? I think there are some, but right now I don't have time to list them here.

desheikh · 2019-09-11T15:27:31Z

Added additional references to todo where relevant 👍.

jonas054

Looks good! 👍

bbatsov · 2019-09-15T07:22:33Z

CHANGELOG.md

@@ -59,6 +59,7 @@
 * [#7215](https://github.com/rubocop-hq/rubocop/issues/7215): Make it clear what's wrong in the message from `Style/GuardClause`. ([@jonas054][])
 * [#7245](https://github.com/rubocop-hq/rubocop/pull/7245): Make cops detect string interpolations in more contexts: inside of backticks, regular expressions, and symbols. ([@buehmann][])
 * Deprecate the `SafeMode` option. Users will need to upgrade `rubocop-performance` to 1.15.0+ before the `SafeMode` module is removed. ([@rrosenblum][])
+* [#7335](https://github.com/rubocop-hq/rubocop/pull/7335): Add todo as an alias to disable. `--disable-uncorrectable` will now disable cops using `rubocop:todo` instead of `rubocop:disable`. ([@desheikh][])


That should be moved to master.

bbatsov · 2019-09-15T07:24:32Z

manual/configuration.md

@@ -565,6 +565,14 @@ You can also disable *all* cops with
 # rubocop:enable all
 ```

+You can also disable cops using the todo alias.


I'd mention what this is an alias of exactly and what's the value of having it. Currently this section is a bit confusing.

Updated the text

bbatsov · 2019-09-15T07:26:31Z

lib/rubocop/cop/offense.rb

+      def corrected_with_todo
+        @status == :corrected_with_todo
+      end
+      alias corrected_with_todo? corrected_with_todo


I don't get the purpose of this alias.

We want to output [Todo] instead of [Corrected] in the rubocop formatter output.
Having a different corrected state allows us to do that.

The method above this one, corrected, is also aliased but I've done some digging in the history and found that there used to be an instance variable called @corrected with an attr_reader :corrected and then we needed the alias corrected? to get the nicely named predicate. Now that we have a @status instance variable, we could just define corrected? and corrected_with_todo? methods and no aliases.

I guess, though, that the new method should have a comment marking it as part of the public API.

I like that, however perhaps that's more suited to being a separate PR?

OK, I'll merge this one, so you can do a follow-up PR.

bbatsov · 2019-09-15T07:28:01Z

We want to to differentiate between disables made intentionally due to false positives, or that we don't plan on fixing vs disables generated using the --disable-uncorrectable flag, which ideally should be reviewed at some point.

This type of reasoning should be in the docs, otherwise no one will figure it out. :-)

Overall the code looks good, but I've left some small remarks here and there.

bbatsov · 2019-09-16T19:15:48Z

Thanks!

desheikh · 2019-09-16T20:42:19Z

🎉

desheikh force-pushed the rubocop-todo-alias branch 6 times, most recently from 9f77a2d to 7198759 Compare September 8, 2019 06:35

desheikh marked this pull request as ready for review September 8, 2019 06:35

jonas054 requested changes Sep 10, 2019

View reviewed changes

desheikh force-pushed the rubocop-todo-alias branch from 7198759 to 79145bd Compare September 10, 2019 20:53

jonas054 reviewed Sep 11, 2019

View reviewed changes

desheikh force-pushed the rubocop-todo-alias branch from 79145bd to 8fa3f54 Compare September 11, 2019 15:21

desheikh requested a review from jonas054 September 11, 2019 19:09

jonas054 approved these changes Sep 12, 2019

View reviewed changes

bbatsov reviewed Sep 15, 2019

View reviewed changes

Add Todo alias and make --disable-autocorrect mark Cops as Todo

882eb58

desheikh force-pushed the rubocop-todo-alias branch from 8fa3f54 to 882eb58 Compare September 15, 2019 21:53

bbatsov merged commit 9da708f into rubocop:master Sep 16, 2019

desheikh deleted the rubocop-todo-alias branch September 16, 2019 20:42

roman-dubrovsky mentioned this pull request Oct 1, 2019

Update rubocop requirement from ~> 0.74.0 to >= 0.74, < 0.76 datarockets/ruby-style#100

Merged

touhouota mentioned this pull request Nov 7, 2019

bundle update touhouota/RTS#24

Merged

dgollahon mentioned this pull request Dec 18, 2020

Allow customizing --disable-uncorrectable to output rubocop:disable instead of rubocop:todo #9249

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Todo alias and make --disable-autocorrect mark Cops as Todo #7335

Add Todo alias and make --disable-autocorrect mark Cops as Todo #7335

desheikh commented Sep 8, 2019 •

edited

Loading

desheikh commented Sep 10, 2019

jonas054 left a comment

jonas054 Sep 10, 2019

desheikh Sep 10, 2019

jonas054 Sep 10, 2019

desheikh Sep 10, 2019 •

edited

Loading

jonas054 Sep 11, 2019 •

edited

Loading

jonas054 commented Sep 11, 2019

desheikh commented Sep 11, 2019

jonas054 left a comment

bbatsov Sep 15, 2019

bbatsov Sep 15, 2019

desheikh Sep 15, 2019

bbatsov Sep 15, 2019

desheikh Sep 15, 2019

jonas054 Sep 16, 2019 •

edited

Loading

desheikh Sep 16, 2019

bbatsov Sep 16, 2019

bbatsov commented Sep 15, 2019

bbatsov commented Sep 16, 2019

desheikh commented Sep 16, 2019

Add Todo alias and make --disable-autocorrect mark Cops as Todo #7335

Add Todo alias and make --disable-autocorrect mark Cops as Todo #7335

Conversation

desheikh commented Sep 8, 2019 • edited Loading

desheikh commented Sep 10, 2019

jonas054 left a comment

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

desheikh Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

jonas054 Sep 11, 2019 • edited Loading

Choose a reason for hiding this comment

jonas054 commented Sep 11, 2019

desheikh commented Sep 11, 2019

jonas054 left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas054 Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Sep 15, 2019

bbatsov commented Sep 16, 2019

desheikh commented Sep 16, 2019

desheikh commented Sep 8, 2019 •

edited

Loading

desheikh Sep 10, 2019 •

edited

Loading

jonas054 Sep 11, 2019 •

edited

Loading

jonas054 Sep 16, 2019 •

edited

Loading