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

Support autocorrect for Style/CaseEquality cop #8322

Merged
merged 1 commit into from Jul 18, 2020

Conversation

fatkodima
Copy link
Contributor

No description provided.

@tejasbubane
Copy link
Contributor

@fatkodima Since rubocop 0.88 is released, you should update VersionChanged & move changelog under master.

@fatkodima fatkodima force-pushed the case_equality-autocorrection branch from ea3bd14 to dfee211 Compare July 13, 2020 16:18
@fatkodima
Copy link
Contributor Author

Updated.

@fatkodima fatkodima force-pushed the case_equality-autocorrection branch from dfee211 to c6b2f86 Compare July 13, 2020 18:58
@@ -29,13 +29,20 @@ module Style
# (1..100).include?(7)
# some_string =~ /something/
#
class CaseEquality < Cop
class CaseEquality < Base
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should not be changed:

Suggested change
class CaseEquality < Base
class CaseEquality < Cop

Copy link
Member

Choose a reason for hiding this comment

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

The auto-correction has been added by the new API, I think it is okay to extend Base. Rather, changing the inheritance to Cop will fail tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

@koic I might be a little confused between Base and Cop. I see most cops inherit from Cop, do not extend AutoCorrector and have separate method autocorrect. Which of these is the preferred/new approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

Inheriting from Base is preferred/new approach. Here's the doc: https://docs.rubocop.org/rubocop/v1_upgrade_notes.html

@bbatsov bbatsov merged commit 994fe7c into rubocop:master Jul 18, 2020
child = lhs.children.first
"#{lhs.source}.include?(#{rhs.source})" if child&.range_type?
when :const
"#{rhs.source}.is_a?(#{lhs.source})"
Copy link
Contributor

@knu knu Aug 11, 2020

Choose a reason for hiding this comment

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

Not all constants are class/module instances. Regexps are often bound to a constant, to be used with ===.

I think a better bet is to replace the expression only if the LHS constant is CamelCased.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your feedback. I've opened the PR #8526.

def replacement(lhs, rhs)
case lhs.type
when :regexp
"#{rhs.source} =~ #{lhs.source}"
Copy link
Contributor

Choose a reason for hiding this comment

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

/regexp/ === object and object =~ /regexp/ are not interchangeable. The former returns true or false whereas the latter returns an index or nil.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly! I've opened the PR #8527.

koic added a commit to koic/rubocop that referenced this pull request Aug 17, 2020
Follow rubocop#8322 (comment)

This PR prevents an incorrect auto-correction for `Style/CaseEquality` cop
when comparing with `===` against a regular expression receiver.

OTOH, the automatic correction from `a === b` to `a.match?(b)` needs to
consider `Regexp.last_match?`, `$~`, `$1`, and etc.
This correction is expected to be supported by `Performance/Regexp` cop.
See: rubocop/rubocop-performance#152
koic added a commit to koic/rubocop that referenced this pull request Sep 14, 2020
Follow rubocop#8322 (comment)

Fix a false positive for `Style/CaseEquality` cop when
the receiver is not a camel cased constant.

The following constant should not be replaced with `is_a?`.
What kind of object is actually assigned depends on the constant. e.g.:

```ruby
REGEXP_CONSTANT === something #=> does not register an offense.
```

This PR will only consider camel-cased constant as class name. e.g.:

```ruby
Array === something #=> auto-correct to `something.is_a?(Array)`
```
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.

None yet

6 participants