Fix false positive for `Style/EmptyLineAfterGuardClause` #5679

koic · 2018-03-14T10:45:24Z

This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is before rescue or ensure.

Reproduction code

def foo
  begin
    return another_object if something_different?
  rescue SomeException
    bar
  end
end

and

def foo
  begin
    return another_object if something_different?
  ensure
    bar
  end
end

Expected behavior

No offenses.

Actual behavior and Steps to reproduce the problem

% rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause
Inspecting 1 file
C

Offenses:

/tmp/example.rb:3:5: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause.
    return another_object if something_different?
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

RuboCop version

% rubocop -V
0.53.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17)

This PR fixes the above false positive.
Also, the condition node.parent.single_line? seems to have no effect on behavior, so this PR removes it.

cc @unkmas

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.

unkmas · 2018-03-14T11:33:51Z

lib/rubocop/cop/style/empty_line_after_guard_clause.rb

@@ -68,6 +68,10 @@ def next_line_empty?(node)
          processed_source[node.last_line].blank?
        end

+        def next_line_rescue_or_ensure?(node)


Method name seems a bit strange to me. It talks about "next line", but receiving node (but actually - current node parent) and checks its type. I think it's better either rename method or pass node instead of its parent.

Thanks for the review. Since I cannot hit upon a good method name, I changed to pass the node 💦

I also failed to find better name, so I just wrote "rename" 😆

This PR fixes a false positive for `Style/EmptyLineAfterGuardClause` when guard clause is before `rescue` or `ensure`. ## Reproduction code ```ruby def foo begin return another_object if something_different? rescue SomeException bar end end ``` and ```ruby def foo begin return another_object if something_different? ensure bar end end ``` ## Expected behavior No offenses. ## Actual behavior and Steps to reproduce the problem ```console % rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause Inspecting 1 file C Offenses: /tmp/example.rb:3:5: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause. return another_object if something_different? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` ## RuboCop version ```console % rubocop -V 0.53.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17) ``` This PR fixes the above false positive. Also, the condition `node.parent.single_line?` seems to have no effect on behavior, so this PR removes it.

bbatsov · 2018-03-16T01:59:20Z

👍

Related rubocop#5679. This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is before `else`. ## Reproduction code ```ruby def foo if cond return another_object if something_different? else bar end end ``` ## Expected behavior No offenses. ## Actual behavior and Steps to reproduce the problem ```console % rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause Inspecting 1 file C Offenses: /tmp/example.rb:3:5: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause. return another_object if something_different? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` ## RuboCop version ```console % rubocop -V 0.53.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17) ``` This PR fixes the above false positive.

Related #5679. This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is before `else`. ## Reproduction code ```ruby def foo if cond return another_object if something_different? else bar end end ``` ## Expected behavior No offenses. ## Actual behavior and Steps to reproduce the problem ```console % rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause Inspecting 1 file C Offenses: /tmp/example.rb:3:5: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause. return another_object if something_different? ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` ## RuboCop version ```console % rubocop -V 0.53.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17) ``` This PR fixes the above false positive.

Related rubocop#5679. This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is after heredoc. ## Reproduction code ```ruby def foo raise ArgumentError, <<-MSG unless path Must be called with mount point MSG bar end ``` :memo: I found this false positivity below. https://github.com/rails/rails/blob/v5.2.0.rc2/actionpack/lib/action_dispatch/routing/mapper.rb#L614-L622 ## Expected behavior No offenses. ## Actual behavior and Steps to reproduce the problem ```console % rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause Inspecting 1 file C Offenses: /tmp/example.rb:2:3: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause. raise ArgumentError, <<-MSG unless path ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` ## RuboCop version ```console % rubocop -V 0.54.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17) ``` This PR fixes the above false positive.

Related #5679. This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is after heredoc. ## Reproduction code ```ruby def foo raise ArgumentError, <<-MSG unless path Must be called with mount point MSG bar end ``` :memo: I found this false positivity below. https://github.com/rails/rails/blob/v5.2.0.rc2/actionpack/lib/action_dispatch/routing/mapper.rb#L614-L622 ## Expected behavior No offenses. ## Actual behavior and Steps to reproduce the problem ```console % rubocop /tmp/example.rb --only Style/EmptyLineAfterGuardClause Inspecting 1 file C Offenses: /tmp/example.rb:2:3: C: Style/EmptyLineAfterGuardClause: Add empty line after guard clause. raise ArgumentError, <<-MSG unless path ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected ``` ## RuboCop version ```console % rubocop -V 0.54.0 (using Parser 2.5.0.4, running on ruby 2.5.0 x86_64-darwin17) ``` This PR fixes the above false positive.

This PR enables `Layout/EmptyLineAfterGuardClause` cop by default. This cop has been introduced by rubocop#5522, with setting that is disabled by default. After that, I improved this cop by rubocop#5679, rubocop#5700, rubocop#5720, and rubocop#5760. I think that this cop can be enabled by default as the background of the issue rubocop#5376. And this PR applies auto-correction using this cop.

This PR enables `Layout/EmptyLineAfterGuardClause` cop by default. This cop has been introduced by #5522, with setting that is disabled by default. After that, I improved this cop by #5679, #5700, #5720, and #5760. I think that this cop can be enabled by default as the background of the issue #5376. And this PR applies auto-correction using this cop.

koic force-pushed the fix_false_positive_empty_line_after_guard_clause branch 2 times, most recently from 2f5732f to c12f736 Compare March 14, 2018 10:52

unkmas reviewed Mar 14, 2018

View reviewed changes

koic force-pushed the fix_false_positive_empty_line_after_guard_clause branch 2 times, most recently from 7670090 to 960b396 Compare March 15, 2018 03:55

koic force-pushed the fix_false_positive_empty_line_after_guard_clause branch from 960b396 to cfaf2f9 Compare March 15, 2018 10:07

bbatsov merged commit b885f38 into rubocop:master Mar 16, 2018

koic deleted the fix_false_positive_empty_line_after_guard_clause branch March 16, 2018 02:35

koic mentioned this pull request Mar 19, 2018

Fix false positive for Style/EmptyLineAfterGuardClause #5700

Merged

8 tasks

dependencies bot mentioned this pull request Mar 22, 2018

Update rubocop in / from 0.52.1 to 0.54.0 chauncey-garrett/dotfiles#106

Open

koic mentioned this pull request Mar 24, 2018

Fix false positive for Style/EmptyLineAfterGuardClause #5720

Merged

8 tasks

koic mentioned this pull request Aug 30, 2018

Enable Layout/EmptyLineAfterGuardClause cop by default #6235

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false positive for `Style/EmptyLineAfterGuardClause` #5679

Fix false positive for `Style/EmptyLineAfterGuardClause` #5679

koic commented Mar 14, 2018 •

edited

Loading

unkmas Mar 14, 2018 •

edited

Loading

koic Mar 14, 2018

unkmas Mar 14, 2018

bbatsov commented Mar 16, 2018

Fix false positive for Style/EmptyLineAfterGuardClause #5679

Fix false positive for Style/EmptyLineAfterGuardClause #5679

Conversation

koic commented Mar 14, 2018 • edited Loading

Reproduction code

Expected behavior

Actual behavior and Steps to reproduce the problem

RuboCop version

unkmas Mar 14, 2018 • edited Loading

Choose a reason for hiding this comment

koic Mar 14, 2018

Choose a reason for hiding this comment

unkmas Mar 14, 2018

Choose a reason for hiding this comment

bbatsov commented Mar 16, 2018

Fix false positive for `Style/EmptyLineAfterGuardClause` #5679

Fix false positive for `Style/EmptyLineAfterGuardClause` #5679

koic commented Mar 14, 2018 •

edited

Loading

unkmas Mar 14, 2018 •

edited

Loading