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 #5388] Add new Style/UnlessMultipleConditions cop #5400

Closed
wants to merge 2 commits into from

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented Jan 5, 2018

Fixes #5388

This cop checks that unless is not used with multiple conditions.
In general, using multiple conditions with unless reduces readability.

However, RuboCop itself doesn't use this cop because it currently supports Ruby 2.1 and 2.2.
In Ruby version not implementing Hash#dig, the following code is useful.

return unless hash[:a] && hash[:a][:b]

Considering the above example, should this cop be disabled by default?


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

Fixes rubocop#5388

This cop checks that `unless` is not used with multiple conditions.
In general, using multiple conditions with `unless` reduces
readability.
Because RuboCop currently supports Ruby 2.1.
In Ruby 2.1 not implementing `Hash#dig`, the following code is useful.

```ruby
return unless hash[:a] && hash[:a][:b]
```
@bbatsov
Copy link
Collaborator

bbatsov commented Jan 9, 2018

I've got one remark about naming - technically you always have a single condition expression, but it might be "composite"/"complex" when composed from several expressions.

I also wonder how this play with the existing cop suggesting the use of unless when possible. I guess it should be configurable to suggest the replacement only in non-composite scenarios.

@wata727
Copy link
Contributor Author

wata727 commented Jan 10, 2018

I've got one remark about naming - technically you always have a single condition expression, but it might be "composite"/"complex" when composed from several expressions.

Maybe, that would be right. However, if the conditional expression is single, In the "style" it is not a problem.
If this name was something like Style/UnlessComplexCondtions, that would be a problem.

I also wonder how this play with the existing cop suggesting the use of unless when possible. I guess it should be configurable to suggest the replacement only in non-composite scenarios.

Umm, this is a very difficult problem...
The usage of unless or if should be selected by a user according to a meaning of conditions, and I guess that RuboCop cannot propose the usage mechanically.

I have to think more about these problems. So, I will close this pull request once. Thank you for your opinion!

@wata727 wata727 closed this Jan 10, 2018
koic added a commit to koic/rubocop that referenced this pull request Jan 8, 2019
flip-flop operator is deprecated since Ruby 2.6.0.

> The flip-flop syntax is deprecated. [Feature rubocop#5400]

- https://github.com/ruby/ruby/blob/v2_6_0/NEWS#language-changes
- https://bugs.ruby-lang.org/issues/5400

```console
% cat example.rb
(1..20).each do |x|
  puts x if (x == 5) .. (x == 10)
end

% ruby -v
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin17]

% ruby example.rb
example.rb:2: warning: flip-flop is deprecated
5
6
7
8
9
10
```

This PR tweaks document and offense message for `Style/FlipFlop` cop.
It will more clarify the reason for checking flip-flop operator.
bbatsov pushed a commit that referenced this pull request Jan 8, 2019
flip-flop operator is deprecated since Ruby 2.6.0.

> The flip-flop syntax is deprecated. [Feature #5400]

- https://github.com/ruby/ruby/blob/v2_6_0/NEWS#language-changes
- https://bugs.ruby-lang.org/issues/5400

```console
% cat example.rb
(1..20).each do |x|
  puts x if (x == 5) .. (x == 10)
end

% ruby -v
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin17]

% ruby example.rb
example.rb:2: warning: flip-flop is deprecated
5
6
7
8
9
10
```

This PR tweaks document and offense message for `Style/FlipFlop` cop.
It will more clarify the reason for checking flip-flop operator.
koic added a commit to koic/rubocop that referenced this pull request Jan 8, 2019
Follow up of rubocop#6635 (comment).

flip-flop operator is deprecated since Ruby 2.6.0.

> The flip-flop syntax is deprecated. [Feature rubocop#5400]

- https://github.com/ruby/ruby/blob/v2_6_0/NEWS#language-changes
- https://bugs.ruby-lang.org/issues/5400

This PR moves `FlipFlop` cop from `Style` department to `Lint` department.
bbatsov pushed a commit that referenced this pull request Jan 11, 2019
Follow up of #6635 (comment).

flip-flop operator is deprecated since Ruby 2.6.0.

> The flip-flop syntax is deprecated. [Feature #5400]

- https://github.com/ruby/ruby/blob/v2_6_0/NEWS#language-changes
- https://bugs.ruby-lang.org/issues/5400

This PR moves `FlipFlop` cop from `Style` department to `Lint` department.
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