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

Fix value omission false positive in Style/MethodCallWithArgsParentheses #11709

Merged
merged 2 commits into from Mar 21, 2023

Conversation

gsamokovarov
Copy link
Contributor

Omitting the parentheses in cases like the example below is ambigious, we don't need to enforce their omission in the EnforcedStyle: omit_parentheses

var =
  unless object.action(value:, other:)
    condition || other_condition
  end

If the parentheses in the condition above are omitted, the semantic of
the code is changed:

var =
  unless object.action value:, other:
    condition || other_condition
  end

# is interpreted as:

var =
  unless object.action value:, other: condition || other_condition
    # empty branch
  end

We should allow parentheses in value omissions in conditional nodes.

Omitting the parentheses in cases like the example below is ambigious,
we don't need to enforce their omission in the `EnforcedStyle: omit_parentheses`

```ruby
var =
  unless object.action(value:, other:)
    condition || other_condition
  end
```

If the parentheses in the condition above are omitted, the semantic of
the code is changed:

```ruby
var =
  unless object.action value:, other:
    condition || other_condition
  end

# is interpreted as:

var =
  unless object.action value:, other: condition || other_condition
    # empty branch
  end
```

We should allow parentheses in value omissions in conditional nodes.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 18, 2023

I think all such cases where the parentheses are required should also be listed somewhere in the documentation examples.

@gsamokovarov
Copy link
Contributor Author

gsamokovarov commented Mar 18, 2023

Technically, the parentheses are not required here but omitting them changes the behaviour of the code. There is ambiguity, which should be left to the developer to decide. I found this case by a crash that I fix in #11712.

Documentation about all those cases can be useful so I will write some. Should it be in this PR as another commit?

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 18, 2023

Another commit in this PR would be fine.

Technically, the parentheses are not required here but omitting them changes the behaviour of the code. There is ambiguity, which should be left to the developer to decide. I found this case by a crash that I fix in #11712.

Yeah, yeah - I get this. This particular case should be covered by some generic comment that we don't require/omit parentheses if this is going to change the meaning of the code. Something like a guiding principle for the cop in general.

@gsamokovarov
Copy link
Contributor Author

image

I have also cleaned up the examples and added more examples where parentheses are required.

# because of the following issue: https://bugs.ruby-lang.org/issues/18396.
# NOTE: The style of `omit_parentheses` still allows parentheses in cases
# where omitting them results in ambiguous or syntactically incorrect
# code. For example, parentheses are required around a method with
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd better to list those cases as bullet points - it will read nicer IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I be exhaustive, though? Those are just a few cases, I can try and present all(most) of the cases here but the list will be quite long.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm on the fence about this. Given how often we find problems with this cop I'm leaning towards more documentation, so it's clearer how it's supposed to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

☝️?

@bbatsov bbatsov merged commit 1ff7d1b into rubocop:master Mar 21, 2023
11 checks passed
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 21, 2023

The updated docs look great. Thanks!

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

2 participants