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

Jump to bottom

Migration cop says it autocorrects, but it doesn't if the comment is enable #7814

Closed
tas50 opened this issue Mar 23, 2020 · 4 comments · Fixed by #7818
Closed

Migration cop says it autocorrects, but it doesn't if the comment is enable #7814

tas50 opened this issue Mar 23, 2020 · 4 comments · Fixed by #7818
Labels
bug

Comments

@tas50
Copy link
Contributor

tas50 commented Mar 23, 2020

Expected behavior

Cop should actually autocorrect rubocop comment

Actual behavior

RuboCop shows that it autocorrects the code, but it does not.

/Users/tsmith/dev/the_world2/cisco-cookbook/files/default/vendor/gems/cisco_node_utils-1.7.0/spec/spec_helper.rb:90:20: C: [Corrected] Migration/DepartmentName: Department name is missing.
  # rubocop:enable Style:BlockComments
                   ^^^^^

Steps to reproduce the problem

Try to autocorrect this code:

# rubocop:disable Style/BlockComments
=begin
  # Stuff
=end
# rubocop:enable Style:BlockComments

RuboCop version

master
@tas50 tas50 changed the title Migration cop says it's autocorrect, but it doesn't if the cop is enable Migration cop says it autocorrects, but it doesn't if the comment is enable Mar 23, 2020
@koic
Copy link
Member

koic commented Mar 23, 2020

This would be a false positive because DepartmentName:CopName is an unexpected format in the disabled comment. So, I think it should not be offensed, not auto-correction.

@koic koic added the bug label Mar 23, 2020
@tejasbubane
Copy link
Contributor

DepartmentName:CopName is an unexpected format

Using DepartmentName:CopName does not disable the cop. So I think Migration/DepartmentName should catch this typo & flag offense to let user know the copname is wrong?

Maybe we could improve message to something like did you mean DepartmentName/CopName?

@koic
Copy link
Member

koic commented Mar 24, 2020

did you mean DepartmentName/CopName ?

This is interesting. On the other hand, this cop's role is to complement a department name. Including typo detection in this cop could be a bit more complicated. IMHO, perhaps it would be better to provide a different feature for typo detection.

@koic koic mentioned this issue Mar 24, 2020
Merged
8 tasks
koic added a commit to koic/rubocop that referenced this issue Mar 24, 2020
@koic
[Fix rubocop#7814] Fix a false positive for Migrate/DepartmentName cop
cf3a020 
### Summary

Fixes rubocop#7814.

This PR fixes a false positive for `Migrate/DepartmentName` cop
when inspecting an unexpected disable comment format.

e.g. `# rubocop:disable Style:BlockComments`

The above expected format is `# rubocop:disable Style/BlockComments`.

### Other Information

`Migration/DepartmentName` cop's role is to complement a department name.
The role would be simple if another feature could detect unexpected
disable comment format.
bbatsov pushed a commit that referenced this issue Mar 24, 2020
@koic @bbatsov
[Fix #7814] Fix a false positive for Migrate/DepartmentName cop
ca80c31 
### Summary

Fixes #7814.

This PR fixes a false positive for `Migrate/DepartmentName` cop
when inspecting an unexpected disable comment format.

e.g. `# rubocop:disable Style:BlockComments`

The above expected format is `# rubocop:disable Style/BlockComments`.

### Other Information

`Migration/DepartmentName` cop's role is to complement a department name.
The role would be simple if another feature could detect unexpected
disable comment format.
@tas50
Copy link
Contributor Author

tas50 commented Mar 24, 2020

Thanks for the quick fix @koic

@tas50 tas50 mentioned this issue Apr 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #7814] Fix a false positive for Migrate/DepartmentName cop koic/rubocop
3 participants
@koic @tejasbubane @tas50