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 false positive for Style/EmptyLineAfterGuardClause #5720

Conversation

koic
Copy link
Member

@koic koic commented Mar 24, 2018

Related #5679.

This PR fixes a false positive for Style/EmptyLineAfterGuardClause when guard clause is after heredoc.

Reproduction code

def foo
  raise ArgumentError, <<-MSG unless path
    Must be called with mount point
  MSG

  bar
end

📝 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

% 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

% 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.


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.

@koic koic force-pushed the fix_a_false_positive_empty_line_after_guard_clause branch 2 times, most recently from 2efaf18 to cf29667 Compare March 24, 2018 07:37
@pocke
Copy link
Collaborator

pocke commented Mar 24, 2018

I guess thsi cop also allows the following code.

def foo
  raise ArgumentError, <<-MSG unless path
    Must be called with mount point
  MSG
  bar
end

Should the cop allow it? I guess the cop should add an offense to the code since there are no new lines between the guard clause and the next statement. But it is an exceptional case, so I'm not sure should the cop do.

I think we should decide the behaviour, and add a test case for this code.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 25, 2018

Guard clauses after heredocs are extremely hard to spot IMO. I wonder if it would be wise to discourage those.

As for this example - it seems to me there should be some blank line in this case.

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 27, 2018

I guess the cop should add an offense to the code since there are no new lines between the guard clause and the next statement.

I agree.

@Drenmi
Copy link
Collaborator

Drenmi commented Mar 27, 2018

I also noticed this cop has been placed in the wrong namespace. Should be in Layout with the other EmptyLine* cops. 🙂

@koic
Copy link
Member Author

koic commented Mar 30, 2018

I also think the example code @pocke showed is false negative. I'll look it in detail later.
Apart from that, this PR shows the value to fix the problem that autocorrect breaks heredoc.

I also noticed this cop has been placed in the wrong namespace. Should be in Layout with the other EmptyLine* cops.

Yep. I think so too. Certainly this cop will not be Style department but Layout department 👍
However, it is different from the purpose I'd like to accomplish by this PR. So it will be another PR.

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.
@koic koic force-pushed the fix_a_false_positive_empty_line_after_guard_clause branch from cf29667 to b4a79d5 Compare March 31, 2018 22:23
@bbatsov bbatsov merged commit a329f75 into rubocop:master Apr 3, 2018
@koic koic deleted the fix_a_false_positive_empty_line_after_guard_clause branch April 3, 2018 04:54
koic added a commit to koic/rubocop that referenced this pull request Apr 10, 2018
Follow up of rubocop#5720 (comment).

This PR fixes incorrect offense location for `Style/EmptyLineAfterGuardClause` when
guard clause is after heredoc argument.

## Reproduction code

```ruby
def foo
  raise ArgumentError, <<-MSG unless path
    Must be called with mount point
  MSG
  bar
end
```

## Expected behavior

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

Offenses:

/tmp/example.rb:4:1: C: Style/EmptyLineAfterGuardClause: Add empty line
after guard clause.
  MSG
^^^^^

1 file inspected, 1 offense detected
```

## Actual behavior and Steps to reproduce the problem

```
% 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.5, running on ruby 2.5.1 x86_64-darwin17)
```

Also, this PR will fix bugs included in changes at rubocop#5720.
bbatsov pushed a commit that referenced this pull request Apr 11, 2018
Follow up of #5720 (comment).

This PR fixes incorrect offense location for `Style/EmptyLineAfterGuardClause` when
guard clause is after heredoc argument.

## Reproduction code

```ruby
def foo
  raise ArgumentError, <<-MSG unless path
    Must be called with mount point
  MSG
  bar
end
```

## Expected behavior

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

Offenses:

/tmp/example.rb:4:1: C: Style/EmptyLineAfterGuardClause: Add empty line
after guard clause.
  MSG
^^^^^

1 file inspected, 1 offense detected
```

## Actual behavior and Steps to reproduce the problem

```
% 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.5, running on ruby 2.5.1 x86_64-darwin17)
```

Also, this PR will fix bugs included in changes at #5720.
koic added a commit to koic/rubocop that referenced this pull request Apr 11, 2018
I intended to wrote rubocop#5720 in "Bug fixes" section, but it probably
moved to an unintended "Changes" section at the timing of rebasing.
bbatsov pushed a commit that referenced this pull request Apr 11, 2018
I intended to wrote #5720 in "Bug fixes" section, but it probably
moved to an unintended "Changes" section at the timing of rebasing.
koic added a commit to koic/rubocop that referenced this pull request May 14, 2018
Follow up of rubocop#5720 (comment).

This PR moves `Style/EmptyLineAfterGuardClause` cop to `Layout` department.

I remember about the proposal that this cop name
`EmptyLinesAroundGuardClause` is good, however I'm not sure about it.
On the other hand, since I think that this cop should be in the `Layout`
department, so this PR moves the department.
bbatsov pushed a commit that referenced this pull request May 14, 2018
Follow up of #5720 (comment).

This PR moves `Style/EmptyLineAfterGuardClause` cop to `Layout` department.

I remember about the proposal that this cop name
`EmptyLinesAroundGuardClause` is good, however I'm not sure about it.
On the other hand, since I think that this cop should be in the `Layout`
department, so this PR moves the department.
koic added a commit to koic/rubocop that referenced this pull request Aug 30, 2018
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.
bbatsov pushed a commit that referenced this pull request Aug 30, 2018
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.
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.

4 participants