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

Make Style/GuardClause a bit more lenient when the replacement would make the code more verbose #10740

Merged
merged 1 commit into from Jun 24, 2022

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Jun 22, 2022

We currently have an exception in Style/GuardClause when the suggested replacement would be too long.

def test
  if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line
    work
  end
end

RuboCop suggests replacing this with:

def test
  unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line
    return
  end
  work
end

However, this makes the code more verbose (adds a line that doesn't really add any benefit) and harder to understand (since this only happens when the line is too long, it's a safe assumption that the condition is long, and flipping it with unless adds cognitive overhead).

This exception is not covered in the style guide either, so it should be "non-breaking".

My proposal here is that if the guard clause is too line but what is inside the if is trivial (a single statement, and not a nested if), it will not be registered as an offense.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@dvandersluis dvandersluis requested a review from bbatsov Jun 22, 2022
def opposite_keyword(node)
node.if? ? 'unless' : 'if'
end
Copy link
Member Author

@dvandersluis dvandersluis Jun 22, 2022

Choose a reason for hiding this comment

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

This is not strictly related to my change but I noticed it could be replaced with node.inverse_keyword.

https://github.com/rubocop/rubocop-ast/blob/816dfe7f2ca4e92c7eda226a9e8b44aa9fa81e81/lib/rubocop/ast/node/if_node.rb#L61-L73

@dvandersluis dvandersluis force-pushed the style/guard-clause branch 2 times, most recently from dfe791a to ebe6d46 Compare Jun 22, 2022
@bbatsov bbatsov merged commit 1c25af3 into rubocop:master Jun 24, 2022
38 checks passed
@bbatsov
Copy link
Collaborator

@bbatsov bbatsov commented Jun 24, 2022

That's an improvement for sure, but overall I think that's one of those cops that need more work and have to be relaxed a bit to focus on cases where the guard clause really improves something.

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