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

New cop Style/SafeNavigation #3453

Merged
merged 2 commits into from Sep 14, 2016

Conversation

Projects
None yet
3 participants
@rrosenblum
Contributor

rrosenblum commented Aug 29, 2016

This addresses #2428.

I am just about done with this new cop. Functionally, I think that I am set. I have run into a name spacing issue though. The name spacing issue is the root of the test failures. qualified_cop_name does not like that there are 2 cops with the same base name (Style/SafeNavigation and Rails/SafeNavigation). I am not sure how we would like to handle this.

The solutions that I have considered are:

  1. Change the name of either the Style/SafeNavigation or Rails/SafeNavigation cop.

I am unsure of a good name to change either of these to. If anyone has ideas, please let me know.

  1. Modify the qualified_cop_name method to do a lookup using the cop type as well as the name of the cop.

This would fix occurrences that use the full cop name including the group. It would not fix occurrences where only the cop name is given. For example, how would we handle rubocop --only SafeNavigation? Should that run both the Rails and Style cop? This would also impact using the comment disable of a cop.

Show outdated Hide outdated config/enabled.yml Outdated
@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Aug 30, 2016

Collaborator

I am unsure of a good name to change either of these to. If anyone has ideas, please let me know.

Modify the qualified_cop_name method to do a lookup using the cop type as well as the name of the cop.
This would fix occurrences that use the full cop name including the group. It would not fix occurrences where only the cop name is given. For example, how would we handle rubocop --only SafeNavigation? Should that run both the Rails and Style cop? This would also impact using the comment disable of a cop.

I'd simply raise some error in such cases where some ambiguity exists.

Collaborator

bbatsov commented Aug 30, 2016

I am unsure of a good name to change either of these to. If anyone has ideas, please let me know.

Modify the qualified_cop_name method to do a lookup using the cop type as well as the name of the cop.
This would fix occurrences that use the full cop name including the group. It would not fix occurrences where only the cop name is given. For example, how would we handle rubocop --only SafeNavigation? Should that run both the Rails and Style cop? This would also impact using the comment disable of a cop.

I'd simply raise some error in such cases where some ambiguity exists.

@rrosenblum rrosenblum changed the title from New cop Style/SafeNavigation to [WIP] - New cop Style/SafeNavigation Aug 31, 2016

@rrosenblum rrosenblum changed the title from [WIP] - New cop Style/SafeNavigation to New cop Style/SafeNavigation Sep 8, 2016

@rrosenblum

This comment has been minimized.

Show comment
Hide comment
@rrosenblum

rrosenblum Sep 8, 2016

Contributor

I finally finished up this cop after having coded it incorrectly the first time.

The cop will check for and correct code in the following formats:

foo.bar if foo 
foo.bar if !foo.nil?
foo.bar unless !foo
foo.bar unless foo.nil?

foo && foo.bar
!foo.nil? &&
foo.nil? || foo.bar
!foo || foo.bar

As with the Rails/SafeNavigation cop, this cop will not register an offense if the method be called is a method that nil responds to. It also will not register an offense for a method that looks weird when combined with &..

The cop name qualifier now checks for an exact match to the cop, including the group, before checking in the existing manner with the cop name only. This is so that we can have 2 cops with the same name in 2 different groups. If a cop name is found to be ambiguous, the names of the matching cops will be printed out in the error message. This also means that we finally get to enable the test for ambiguous cop names.

Contributor

rrosenblum commented Sep 8, 2016

I finally finished up this cop after having coded it incorrectly the first time.

The cop will check for and correct code in the following formats:

foo.bar if foo 
foo.bar if !foo.nil?
foo.bar unless !foo
foo.bar unless foo.nil?

foo && foo.bar
!foo.nil? &&
foo.nil? || foo.bar
!foo || foo.bar

As with the Rails/SafeNavigation cop, this cop will not register an offense if the method be called is a method that nil responds to. It also will not register an offense for a method that looks weird when combined with &..

The cop name qualifier now checks for an exact match to the cop, including the group, before checking in the existing manner with the cop name only. This is so that we can have 2 cops with the same name in 2 different groups. If a cop name is found to be ambiguous, the names of the matching cops will be printed out in the error message. This also means that we finally get to enable the test for ambiguous cop names.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 9, 2016

Collaborator

Great work! Rebase this and let's have it merged.

Collaborator

bbatsov commented Sep 9, 2016

Great work! Rebase this and let's have it merged.

@bbatsov

This comment has been minimized.

Show comment
Hide comment
@bbatsov

bbatsov Sep 13, 2016

Collaborator

The build is failing.

Collaborator

bbatsov commented Sep 13, 2016

The build is failing.

@rrosenblum

This comment has been minimized.

Show comment
Hide comment
@rrosenblum

rrosenblum Sep 14, 2016

Contributor

The build is now passing.

Contributor

rrosenblum commented Sep 14, 2016

The build is now passing.

@bbatsov bbatsov merged commit a66449a into rubocop-hq:master Sep 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rrosenblum rrosenblum deleted the rrosenblum:safe_navigation branch Sep 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment