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

Add new Style/SoleNestedConditional cop #8390

Merged
merged 1 commit into from Aug 22, 2020

Conversation

fatkodima
Copy link
Contributor

This cop checks for places where nested conditionals can be merged into outer ones, like:

# bad
if condition_a
  if condition_b
    do_something
  end
end

# good
if condition_a && condition_b
  do_something
end

This, imo, makes code more readable and, for sure, reduces nestings.

Inspired by Style/IfInsideElse cop.
That cop also has (disabled, btw, in rubocops .rubocop.yml file) AllowIfModifier setting. I haven't added that as I haven't get the point of its usefulness and how this

if condition_a
  do_something if condition_b
end

can be better than

if condition_a && condition_b
  do_something
end

Probably, the cop name and wordings can be improved.

@marcandre
Copy link
Contributor

I can see how this:

if important_criteria
  do_something(:important, 'with a long argument') unless unlikely_minor_cond?
end

# could be better than
if important_criteria && !unlikely_minor_cond?
  do_something(:important, 'with a long argument')
end

Copy link
Contributor

@marcandre marcandre left a comment

Choose a reason for hiding this comment

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

Also, did you handle the cases if (var = calculate) manually? That's cheating 🤣. I think that's if a criteria is an assignment than no offense should be raised, no?

I don't see any comments in the specs, are these handled correctly? We should always put comments in our specs, as these are typically difficult to handle, especially when there are line changes.

@fatkodima
Copy link
Contributor Author

Also, did you handle the cases if (var = calculate) manually? That's cheating 🤣. I think that's if a criteria is an assignment than no offense should be raised, no?

Not sure. Personally, I'm happy with offense raised in this case 😄 I extracted that cases as a separate assignment before conditional, but would also just append nested conditions to that assignment. I looks more simpler/clear to me as a separate line.

@fatkodima
Copy link
Contributor Author

Added AllowModifier option and test with comment.

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 22, 2020

The changes look good, but your branch has to be rebased on top of the current master branch due to merge conflicts.

@fatkodima
Copy link
Contributor Author

Rebased.

@bbatsov bbatsov merged commit 4727abf into rubocop:master Aug 22, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 22, 2020

Thanks!

koic added a commit to rubocop/rubocop-rails that referenced this pull request Aug 23, 2020
Follow rubocop/rubocop#8390.

```console
% bundle exec rake

Offenses:

lib/rubocop/cop/rails/has_many_or_has_one_dependent.rb:57:20: C:
Style/SoleNestedConditional: Consider merging nested conditions into
outer unless conditions.
            return if valid_options?(association_with_options?(node))
                   ^^

185 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
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

3 participants