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/RedundantParentheses should allow parenthesis for compound logical statements #12511

Open
postmodern opened this issue Dec 4, 2023 · 5 comments · May be fixed by #12892
Open

Style/RedundantParentheses should allow parenthesis for compound logical statements #12511

postmodern opened this issue Dec 4, 2023 · 5 comments · May be fixed by #12892

Comments

@postmodern
Copy link

Recently rubocop began flagging code with the Style/RedundantParentheses rule. However, it was flagging complex compound logical statements where the parenthesis help with showing the beginning and end of said statement, which helps with code readability.

lib/ronin/app/schemas/params_schema.rb:55:14: C: [Correctable] Style/RedundantParentheses: Don't use parentheses around a logical expression.
10
          if (param.required? && !param.has_default?)
11
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12
lib/ronin/app/validations/nmap_params.rb:156:14: C: [Correctable] Style/RedundantParentheses: Don't use parentheses around a logical expression.
13
          if (value && !(value >= 0.0 && value <= 1.0))
14
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15
lib/ronin/app/validations/nmap_params.rb:162:14: C: [Correctable] Style/RedundantParentheses: Don't use parentheses around a logical expression.
16
          if (value && !(value >= 0 && value <= 9))
17
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I do still believe Style/RedundantParentheses can be useful for catching parenthesis around singular logical statements (ex: if (variable.nil?)).

I believe Style/RedundantParentheses should make an exception for compound logical statements, or should at least be configurable to make an exception for compound logical statements. Disabling the rule entirely would allow needless parenthesis around singular logical statements to slip by.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 4, 2023

I do still believe Style/RedundantParentheses can be useful for catching parenthesis around singular logical statements (ex: if (variable.nil?)).

Those are super rare in practice, though, so providing special support for them seems like an overkill to me. Might be best to just implement a custom cop targeting those.

@postmodern
Copy link
Author

@bbatsov if you check the documentation for Style/RedundantParentheses it shows:

# bad
(x) if ((y.z).nil?)

# good
x if y.z.nil?

so I assume the purpose of this cop is to also eliminate parenthesis around singular statements, even though they are super rare. Despite being rare, I'd like to ensure they don't get into my code, while still allowing parenthesis around compound logical statements.

@koic
Copy link
Member

koic commented Dec 5, 2023

In all the cases exemplified, Ruby style does not use parentheses around the outermost part of condition. And This cop respects parentheses that alter the precedence of the condition and targets only the outermost parentheses for removal.
Additionally, this rule is also indicated in the Ruby Style Guide, as shown here:
https://rubystyle.guide/#no-parens-around-condition

@postmodern
Copy link
Author

postmodern commented Dec 5, 2023

@koic

In all the cases exemplified, Ruby style does not use parentheses around the outermost part of condition.
Additionally, this rule is also indicated in the Ruby Style Guide, as shown here:
https://rubystyle.guide/#no-parens-around-condition

The rubocop examples show only invole singular logical statements (ex: if (value.nil?)), not compound logical statements (ex: if (foo && (bar || !baz))).

The examples listed in the Ruby Style guide also only use singular logical statements, not compound logical statements. There is no authoritative style example for whether parenthesis in compound logical statements is allowed or prohibited This should be clarified, as currently every example only refers to singular logical statements.

@seandilda
Copy link

I would like to second this. I've had a couple cases that prompted me to disable this cop entirely because it wanted my code to be less readable. For instance:

@var ||= (method1 || method2)

Ruby does not require the parentheses. However, having the parentheses makes it very clear what is happening in the code. If the parentheses are gone, I expect developers looking at the code later to have to pause and consider if the code actually works the way it is intended.

koic added a commit to koic/rubocop that referenced this issue May 8, 2024
Fixes rubocop#12511 (comment).

This PR makes `Style/RedundantParentheses` allow parentheses around conditional expressions in assignment
to respect user's intentions for readability:

```ruby
var = (foo || bar)
```
@koic koic linked a pull request May 8, 2024 that will close this issue
8 tasks
koic added a commit to koic/rubocop that referenced this issue May 9, 2024
Fixes rubocop#12511 (comment).

This PR makes `Style/RedundantParentheses` allow parentheses around conditional expressions in assignment
to respect user's intentions for readability:

```ruby
var = (foo || bar)
```
koic added a commit to koic/rubocop that referenced this issue May 9, 2024
Fixes rubocop#12511 (comment).

This PR makes `Style/RedundantParentheses` allow parentheses around conditional expressions in assignment
to respect user's intentions for readability:

```ruby
var ||= (foo || bar)
```
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 a pull request may close this issue.

4 participants