Skip to content
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

Tweak Lint/UnneededDisable cop behavior. #4938

Merged
merged 2 commits into from
Nov 5, 2017
Merged

Tweak Lint/UnneededDisable cop behavior. #4938

merged 2 commits into from
Nov 5, 2017

Conversation

tdeo
Copy link
Contributor

@tdeo tdeo commented Oct 25, 2017

I was noticing several weird behaviours with UnneededDisable.

First one is linked to #4459,
it turns out the following piece of code:

  # rubocop:disable Lint/UnneededDisable
  # rubocop:disable all

returns the offense "Unnecessary disabling of all cops." where I believe
it shouldn't. It seems this was introduced by @rrosenblum changes
in UnneededDisable#check in d042ccff

Second point I noticed and wanted to address is that the following code:

  # rubocop:disable Lint/UnneededDisable
  # rubocop:disable Bundler/OrderedGems
  # rubocop:disable Bundler/OrderedGems

produces an offense for unnecessary disabling on line 3. This shouldn't be
reported as the cop is disabled. This commit addresses it as well.


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).
  • Used the same coding conventions as the rest of the project.
  • 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.
  • All tests(rake spec) are passing.
  • The new code doesn't generate RuboCop offenses that are checked by rake internal_investigation.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@rrosenblum
Copy link
Contributor

rrosenblum commented Oct 25, 2017

The commits will need to be squashed.

Linking for context, the commit that you mentioned was introduced in #1967 as a fix for #1959.

Copy link
Contributor

@rrosenblum rrosenblum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.

end

it 'does not return an offense' do
puts cop.offenses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it may have been left in by mistake.

tdeo and others added 2 commits October 27, 2017 01:11
I was noticing several weird behaviours with UnneededDisable.

First one is linked to #4459,
it turns out the following piece of code:
```
  # rubocop:disable Lint/UnneededDisable
  # rubocop:disable all
```
returns the offense "Unnecessary disabling of all cops." where I believe
it shouldn't. It seems this was introduced by @rrosenblum changes
in `UnneededDisable#check` in d042ccff

Second point I noticed and wanted to address is that the following code:
```
  # rubocop:disable Lint/UnneededDisable
  # rubocop:disable Bundler/OrderedGems
  # rubocop:disable Bundler/OrderedGems
```
produces an offense for unnecessary disabling on line 3. This shouldn't be
reported as the cop is disabled. This commit addresses it as well.
@bbatsov bbatsov merged commit dfdbd39 into rubocop:master Nov 5, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Nov 5, 2017

This issue reminded me that we should eventually name the cop UnneedCopDisableDirective or something else more informative.

@tdeo tdeo deleted the unneeded_disable branch November 6, 2017 10:37
schneems added a commit to heroku/heroku-buildpack-ruby that referenced this pull request May 9, 2018
For a backstory see rubocop/rubocop#4459

You can configure rubocop to error out when anyone disables rubocop errors. Which is fine, except the Ruby buildpack needs to do that. We needed a way to disable that check, so this feature was added rubocop/rubocop#4938.

This PR is using that feature to hopefully allow anyone to deploy and not get errors from rubocop.
@schneems
Copy link
Contributor

schneems commented May 10, 2018

I’m getting this error with rubocop 0.55.0 did this feature ship?

lib/tasks/heroku_clear_tasks.rake:1:1: W: Lint/UnneededCopDisableDirective: Unnecessary disabling of Lint/UnneededDisable (unknown cop).
# rubocop:disable Lint/UnneededDisable

@tdeo
Copy link
Contributor Author

tdeo commented May 14, 2018

@schneems Probably because of this commit released in 0.53.0: a0870fb

Changing your statement to # rubocop:disable Lint/UnneededDisable, Lint/UnneededCopDisableDirective should hopefully be both pre-0.53 and post-0.53 compatible

EDIT: I just tested the following code against versions of rubocop, and it works with any version >= 0.52.0:

# rubocop:disable Lint/UnneededDisable, Lint/UnneededCopDisableDirective
# rubocop:disable all

# any code with or without offenses

# rubocop:enable all
# rubocop:enable Lint/UnneededDisable, Lint/UnneededCopDisableDirective

I'm not sure whether this should a core feature of rubocop (being able to ensure no offenses are possible within some code). If so, we should probably add a test that covers this snippet.

schneems added a commit to heroku/heroku-buildpack-ruby that referenced this pull request May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants