Rubocop Issue: Omit parentheses for ternary conditions

[irfan-shaik]

There is a wrong validation in rubocop where you have a function in ternary condition which accepts single param.
Ex:-
error_zip_file = 'testfile/path'
File.exist? (error_zip_file) ? path(error_zip_file) : nil

Expected behavior

It should not show errors for this statement.
Or
It should ask the coder to remove the space between 'exist?' and '(error_zipfile)' with the braces ON.

Actual behavior

It is showing invalid errors for this statement.
Errors:
1). Don't use parentheses around a variable.
2). Omit parentheses for ternary conditions.

Steps to reproduce the problem

error_zip_file = 'testfile/path'
File.exist? (error_zip_file) ? path(error_zip_file) : nil

Run rubocop for the above code.
Expected Output: it should return the path of error_zip_file if its present
If we change the code as per errors then the result will be wrong.
Fixed Code as per Rubocop:
File.exist? error_zip_file ? path(error_zip_file) : nil
with this code, ruby will actually send the rest of the code to File.exist? as a single param.
File.exist? (error_zip_file ? path(error_zip_file) : nil) --- Wrong
Now the result will be true or false --- Wrong.

RuboCop version

$ rubocop -V
0.46.0 (using Parser 2.3.3.1, running on ruby 2.1.5 x86_64-darwin15.0)

[rrosenblum]

If we change the code as per errors then the result will be wrong.
Fixed Code as per Rubocop:
File.exist? error_zip_file ? path(error_zip_file) : nil
with this code, ruby will actually send the rest of the code to File.exist? as a single param.

I actually believe that this is already happening in your code based on the example provided. The reason that RuboCop is registering an offense here is that the Ruby code is being parsed as the ternary statement being the argument to File.exists?

[1] pry(#<RuboCop::Cop::Style::TernaryParentheses>)> node
=> s(:if,
  s(:begin,
    s(:send, nil, :error_zip_file)),
  s(:send, nil, :path,
    s(:send, nil, :error_zip_file)),
  s(:nil))
[2] pry(#<RuboCop::Cop::Style::TernaryParentheses>)> node.source
=> "(error_zip_file) ? path(error_zip_file) : nil"
[3] pry(#<RuboCop::Cop::Style::TernaryParentheses>)> node.parent
=> s(:send,
  s(:const, nil, :File), :exist?,
  s(:if,
    s(:begin,
      s(:send, nil, :error_zip_file)),
    s(:send, nil, :path,
      s(:send, nil, :error_zip_file)),
    s(:nil)))
[4] pry(#<RuboCop::Cop::Style::TernaryParentheses>)> node.parent.source
=> "File.exist? (error_zip_file) ? path(error_zip_file) : nil"

It should ask the coder to remove the space between 'exist?' and '(error_zipfile)' with the braces ON.

This is kind of already happening from Lint/ParenthesesAsGroupedExpression

rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ cat test.rb 
# frozen_string_literal: true

error_zip_file = 'testfile/path'
File.exist? (error_zip_file) ? path(error_zip_file) : nil
rrosenblum@C02NV0KSG3QN:~/work/rubocop (master u=)$ b rubocop -D test.rb
Inspecting 1 file
W

Offenses:

test.rb:4:12: W: Lint/ParenthesesAsGroupedExpression: (...) interpreted as grouped expression.
File.exist? (error_zip_file) ? path(error_zip_file) : nil
           ^
test.rb:4:13: C: Style/RedundantParentheses: Don't use parentheses around a variable.
File.exist? (error_zip_file) ? path(error_zip_file) : nil
            ^^^^^^^^^^^^^^^^
test.rb:4:13: C: Style/TernaryParentheses: Omit parentheses for ternary conditions.
File.exist? (error_zip_file) ? path(error_zip_file) : nil
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 3 offenses detected

[Drenmi]

Hm. It's very hard for RuboCop to know the programmer made a mistake here, because they may just as well have meant:

File.exist? (error_zip_file) ? path(error_zip_file) : nil

to do what it does.

One thing that can be done is enable the Style/MethodCallWithArgsParentheses cop, which will maybe let the programmer realize his mistake.

Possibly we could add a Lint/TernaryArgumentParentheses cop to warn against cases like these.

[irfan-shaik]

@rrosenblum Whatever you mentioned in your comment is fine. But what about the second part of my question where
File.exist? error_zip_file ? path(error_zip_file) : nil
this is being wrongly getting interpreted by ruby and returns true or false???

[mikegee]

@irfu572 in File.exist? error_zip_file ? path(error_zip_file) : nil, Ruby evaluates the ternary (error_zip_file ? path(error_zip_file) : nil) first and its value is given to the method call (File.exist?).

Hopefully this simplification is informative:

def file_exist?(arg)
  "arg = #{arg.inspect}"
end

file_exist? 'error_zip_file' ? 'path_to_error_zip_file' : nil
#=> arg = "path_to_error_zip_file"

file_exist? ('error_zip_file') ? 'path_to_error_zip_file' : nil
#=> arg = "path_to_error_zip_file"

file_exist?('error_zip_file') ? 'path_to_error_zip_file' : nil
#=> arg = "error_zip_file"

Best practice in this case is to use parentheses for the File.exist? call. Those parentheses need to have no space after the method name; the space changes the meaning.

I hope that helps. Let me know if I am unclear.

[irfan-shaik]

@mikegee Thats exactly what I am saying. Rubocop should notify us right when we are unknowingly making this mistake.
When a developer writes this type of code he'll feel that
File.exist? error_zip_file ? path(error_zip_file) : nil
is a ternary condition with the params for File.exist? as 'error_zip_file'.
But in this case the actual params for File.exist? will be 'error_zip_file' ? 'path_to_error_zip_file' : nil'

Ideal Solution for this would be, as you said,
Best practice in this case is to use parentheses for the File.exist? call. Those parentheses need to have no space after the method name; the space changes the meaning.
This thing should be provided by the rubocop for the ternary conditions atleast..
1). To remove the space between the File.exist? and the params provided for it
2). And those params should be given inside the parenthesis.

[rrosenblum]

RuboCop has not added a cop that will check for parentheses because the rules are too vague to handle a generic solution. Checkout the style guide recommendations for parenthesis.

Only omit parentheses for
Methods that are part of an internal DSL (e.g., Rake, Rails, RSpec):

# bad
validates(:name, presence: true)
# good
validates :name, presence: true

There are too many DSLs to be able to know what methods should or shouldn't have parentheses.

Unfortunately, there isn't much that we can do here. For your purpose File.exist? error_zip_file ? path(error_zip_file) : nil is incorrect because you want to be using File.exist?(error_zip_file) for the ternary condition. However we can't make that assumption for everyone. There could be people that are trying to determine which filename to pass to File.exist?. For that reason, we can only make recommendations based on the code that is scanned and how it is interpreted.