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.

Sign up for GitHub

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

Fix a false positive for Style/CaseLikeIf when conditional contains comparison with a class #8511

Merged
merged 1 commit into from Aug 12, 2020
Merged

Fix a false positive for Style/CaseLikeIf when conditional contains comparison with a class #8511

merged 1 commit into from Aug 12, 2020

Conversation

fatkodima
Copy link
Contributor

Closes #8508

@fatkodima fatkodima mentioned this pull request Aug 10, 2020
Closed
marcandre
marcandre reviewed Aug 11, 2020
View reviewed changes
lib/rubocop/cop/style/case_like_if.rb Outdated
def class_reference?(node)
return false unless node.const_type?

name = node.children[1].to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using node.children[1].match? /[[:lower:]]/ instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code checks that const name is not all uppercase, so the code could be written as !name.match?(/^[[:upper:]]+$/), but this does not handle numbers in names and so, the original code looks simpler to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

checks that const name is not all uppercase

Right. I'm sorry though, I don't see how "not all uppercase" != "has a lowercase", could you provide an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, sorry. I mistakenly matched that regexp only with the first letter 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

marcandre
marcandre requested changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix. Could you also mark it as Safe: false as there are many potential cases we're not identifying properly (any object where === and == is not the same, like Set for example...)

@fatkodima fatkodima force-pushed the case_like_if-classes-in-conditions branch from 8ebec06 to f3eb0ee Compare August 11, 2020 07:19
@fatkodima
Copy link
Contributor Author

Marked it as unsafe.

@fatkodima fatkodima force-pushed the case_like_if-classes-in-conditions branch from f3eb0ee to 598f812 Compare August 12, 2020 18:10
@marcandre
Copy link
Contributor

I'll merge after the merge conflict is resolved... 😢

@fatkodima fatkodima force-pushed the case_like_if-classes-in-conditions branch from 598f812 to 8ae6e89 Compare August 12, 2020 19:04
@marcandre marcandre merged commit 636b547 into rubocop:master Aug 12, 2020
@marcandre
Copy link
Contributor

Thanks!

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

@marcandre marcandre marcandre requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bad autocorrect for Style/CaseLikeIf
2 participants
@fatkodima @marcandre