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

Style/CaseLikeIf false positive #8891

Closed
noraj opened this issue Oct 13, 2020 · 7 comments
Closed

Style/CaseLikeIf false positive #8891

noraj opened this issue Oct 13, 2020 · 7 comments
Labels

Comments

@noraj
Copy link

noraj commented Oct 13, 2020

Related issues

#8541, #7736 (comment), and mostly #8508

Expected behavior

No offense when there is a something else than a comparison with a string/int, etc.
This is the same as #8508 where there was a comparison with a class but here it's is_a? method rather than the equal operator.

Actual behavior

False positive when using a method, eg. is_a? with a class.

if arg_feeds[0].is_a?(String)
 #
elsif arg_feeds[0].is_a?(Array)
 #
else
 #
end

Steps to reproduce the problem

$ git clone https://gitlab.com/noraj/nvd_api.git && cd nvd_api
$ gem install bundle
$ ./bin/nvd_feed_api_setup
$ rake install
$ rubocop

image

RuboCop version

$ bundle exec rubocop -V
0.92.0 (using Parser 2.7.1.5, rubocop-ast 0.7.1, running on ruby 2.7.1 x86_64-linux)
@koic
Copy link
Member

koic commented Oct 14, 2020

case statement uses === so it does not look like a false positive to is_a?.
https://ruby-doc.org/core-2.7.0/Module.html#method-i-3D-3D-3D

@noraj
Copy link
Author

noraj commented Oct 14, 2020

is_a? always return true or false.

irb(main):002:0> 1.is_a?(Numeric)
=> true
irb(main):003:0> 1.is_a?(Integer)
=> true

So if you have:

case var
when is_a?(Numeric)
 # command
when is_a?(Integer)
  # command
end

And you pass var = 1, both when statement will in fact be when true, I don't know if both will be executed or if a case-witch always execute only one statement.

I don't exactly understand why but when I switch from if/elsif to switch-case it broke all my unit test when it was using is_a?.

@noraj
Copy link
Author

noraj commented Oct 14, 2020

Take an example like here https://gitlab.com/noraj/nvd_api/-/blob/master/lib/nvd_feed_api.rb#L94-100

and change

if arg_feeds[0].is_a?(String)
elsif arg_feeds[0].is_a?(Array)

into

case arg_feeds[0]
when is_a?(String)
whenis_a?(Array)

And run bundle exec rake install before and after and you'll see before all is good and after nothing work.

@koic
Copy link
Member

koic commented Oct 14, 2020

When auto-correction (rubocop --only Style/CaseLikeIf -A) is executed, it will be as follows.

diff --git a/lib/nvd_feed_api.rb b/lib/nvd_feed_api.rb
index 0c145f5..addbe2e 100644
--- a/lib/nvd_feed_api.rb
+++ b/lib/nvd_feed_api.rb
@@ -91,12 +91,13 @@ class NVDFeedScraper
     if arg_feeds.empty?
       return_value = @feeds
     elsif arg_feeds.length == 1
-      if arg_feeds[0].is_a?(String)
+      case arg_feeds[0]
+      when String
         @feeds.each do |feed| # feed is an object
           return_value = feed if arg_feeds.include?(feed.name)
         end
         # if nothing found return nil
-      elsif arg_feeds[0].is_a?(Array)
+      when Array
         raise 'one of the provided arguments is not a String' unless arg_feeds[0].all? { |x| x.is_a?(String) }

         # Sorting CVE can allow us to parse quicker
@@ -165,7 +166,8 @@ class NVDFeedScraper
     raise 'no argument provided, 1 or more expected' if arg_cve.empty?

     if arg_cve.length == 1
-      if arg_cve[0].is_a?(String)
+      case arg_cve[0]
+      when String
         raise 'bad CVE name' unless /^CVE-[0-9]{4}-[0-9]{4,}$/i.match?(arg_cve[0])

         year = /^CVE-([0-9]{4})-[0-9]{4,}$/i.match(arg_cve[0]).captures[0]
@@ -186,7 +188,7 @@ class NVDFeedScraper
         f = feeds(matched_feed)
         f.json_pull
         return_value = f.cve(arg_cve[0])
-      elsif arg_cve[0].is_a?(Array)
+      when Array
(snip)

So, the expected syntax is:

case var
when Numeric
 # command
when Integer
  # command
end

How about it?
(Maybe it can be improved to the warning message.)

@fatkodima
Copy link
Contributor

Correction:
The autocorrected code will be

case var
when Numeric
 # command
when Integer
  # command
end

case-when executes the body of the first (and only one) truthy condition, like with if-elsif.
So, this is not a false positive.

@noraj
Copy link
Author

noraj commented Oct 17, 2020

So the idea is that arg_feeds[0].is_a?(Array) == true is equivalent to arg_feeds[0].class == Array so we can write when String in both cases. I applied with rubocop --only Style/CaseLikeIf -A and it doesn't break my code, thanks for the clarification.

@marcandre
Copy link
Contributor

So the idea is that arg_feeds[0].is_a?(Array) == true is equivalent to arg_feeds[0].class == Array

is equivalent to Array === arg_feeds[0], and when Array is equivalent to Array ===

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants