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 autocorrection to Lint/EmptyConditionalBody #10862

Merged

Conversation

dvandersluis
Copy link
Member

I noticed in #10858 (comment) that Lint/EmptyConditionalBody does not have autocorrection, so I added it. It does the following:

  • Removes the empty branch, including any comments if AllowComments is false.
  • Handles subsequent branches, including changing elsif to if and else to if/unless appropriately, if the topmost branch is the one being removed.
  • Can handle the if statement being preceded by another node.

I believe I've covered the edge cases here but if there's one you can think of let me know and I'll add it in. I also was unsure if we should mark this as SafeAutoCorrect: false but am happy to do so.


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 force-pushed the lint/empty-conditional-body-autocorrect branch from 89cd694 to 7045023 Compare August 3, 2022 17:44
@dvandersluis dvandersluis force-pushed the lint/empty-conditional-body-autocorrect branch 2 times, most recently from 343f417 to 8e0bdc6 Compare August 4, 2022 04:07
@dvandersluis dvandersluis force-pushed the lint/empty-conditional-body-autocorrect branch from 8e0bdc6 to 05b7f70 Compare August 4, 2022 04:08
@bbatsov bbatsov merged commit cbe9248 into rubocop:master Aug 4, 2022
@dvandersluis dvandersluis deleted the lint/empty-conditional-body-autocorrect branch August 4, 2022 11:49
elsif another_condition
do_something_else
end
RUBY
Copy link
Contributor

Choose a reason for hiding this comment

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

@dvandersluis this correction doesn't have the same semantics. To get the same meaning, it should be:

if !condition && other_condition
  do_something
elsif !condition && another_condition
  do_something_else
end

I don't know if this autocorrection was considered safe before, but it definitely isn't safe or correct now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I noticed this after it was merged, and it's been changed to SafeAutoCorrect: false. I haven't had a chance to look at fixing the autocorrection yet so feel free to create a PR!

x = if bar
5
end
RUBY
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, this should be:

x = if !foo && bar
  5
end

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

4 participants