Navigation Menu

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 a false positive for Layout/ClosingParenthesisIndentation #6351

Conversation

antonzaytsev
Copy link
Contributor

@antonzaytsev antonzaytsev commented Oct 3, 2018

This PR fixes a false negative for Layout/ClosingParenthesisIndentation when first argument is multiline.

The following is a reproduction step.

% rubocop -V
0.59.2 (using Parser 2.5.1.2, running on ruby 2.5.0 x86_64-darwin17)

% cat app/models/users.rb
class User < ApplicationRecord
  def self.complex_find
    where(
      "users.approved_at < ? OR
       users.approved_at IS NULL", 3.days.ago
    )
  end
end

% rubocop app/models/users.rb --only Layout/ClosingParenthesisIndentation -d
or /Users/anton/code/rubocop: configuration from /Users/anton/code/rubocop/.rubocop.yml
configuration from /Users/anton/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rubocop-rspec-1.29.1/config/default.yml
configuration from /Users/anton/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rubocop-rspec-1.29.1/config/default.yml
Default configuration from /Users/anton/code/rubocop/config/default.yml
Inheriting configuration from /Users/anton/code/rubocop/.rubocop_todo.yml
Inspecting 1 file
Scanning /Users/anton/code/rubocop/users.rb
C

Offenses:

users.rb:6:5: C: Layout/ClosingParenthesisIndentation: Indent ) to column 5 (not 4)
    )
    ^

1 file inspected, 1 offense detected
Finished in 0.5669199999974808 seconds

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. It executes all tests and RuboCop for itself, and generates the documentation.

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch 3 times, most recently from ac7771b to 74d9678 Compare October 3, 2018 12:06
@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch 2 times, most recently from 054c5d5 to 4b75173 Compare October 3, 2018 13:36
Copy link
Contributor

@rrosenblum rrosenblum left a comment

Choose a reason for hiding this comment

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

Isn't this fixing a false positive?

@antonzaytsev
Copy link
Contributor Author

@rrosenblum good questions actually. Do you have example handy?

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 4b75173 to b5031e3 Compare October 9, 2018 16:43
@rrosenblum
Copy link
Contributor

I don't have a specific example. In general, I tend to think of a false positive as code that generates an offense when it should not. A false negative is when code doesn't produce an offense, but it should.

@antonzaytsev antonzaytsev changed the title Fix a false negative for Layout/ClosingParenthesisIndentation Fix a false positive for Layout/ClosingParenthesisIndentation Oct 10, 2018
@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from b5031e3 to 403d12e Compare October 10, 2018 04:44
@antonzaytsev
Copy link
Contributor Author

@rrosenblum you're right, it is a fix for false positive. Thanks!

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 403d12e to 9b7b95a Compare October 10, 2018 04:45
Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

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

⛵️

@cgriego
Copy link
Contributor

cgriego commented Oct 24, 2018

I've also run into an issue where the following code did not report an offense in 0.55 but does now. I'm not sure if this will fix it, but seems likely?

  foo = foo.bar(<<-SQL
    CODE HERE
    SQL
  )

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 28, 2018

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

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 5f52d97 to c22e191 Compare October 28, 2018 18:51
@antonzaytsev
Copy link
Contributor Author

@bbatsov rebased, conflict solved.

@cgriego
Copy link
Contributor

cgriego commented Dec 2, 2018

The conflict was fixed in October but since nothing happened afterward, there’s another conflict.

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from c22e191 to 0881efa Compare December 3, 2018 06:49
@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 0881efa to 3daec72 Compare December 14, 2018 07:00
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 14, 2018

@antonzaytsev @cgriego Since we have some releases since then the entry also has to be moved where it belongs.

I'm sorry for the slow turn-around, but I can't always keep up with everything here. In such cases (when it's clear that a PR is a goon shape and it has been vetted by Core Team members) it'd be best if the other @rubocop-hq/rubocop-core members merge the PRs after they've been approved, to avoid the constant rebases and merge delays caused by my lack of bandwidth.

@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 3daec72 to 8ff6db2 Compare December 14, 2018 11:37
@antonzaytsev
Copy link
Contributor Author

@cgriego @bbatsov I rebased with latest master and move record in changelog up into master section.

This PR fixes a false negative for `Layout/ClosingParenthesisIndentation` when first argument is multiline.

The following is a reproduction step.
```bash
% rubocop -V
0.59.2 (using Parser 2.5.1.2, running on ruby 2.5.0 x86_64-darwin17)

% cat app/models/users.rb
class User < ApplicationRecord
  def self.complex_find
    where(
      "users.approved_at < ? OR
       users.approved_at IS NULL", 3.days.ago
    )
  end
end

% rubocop app/models/users.rb --only Layout/ClosingParenthesisIndentation -d
or /Users/anton/code/rubocop: configuration from /Users/anton/code/rubocop/.rubocop.yml
configuration from /Users/anton/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rubocop-rspec-1.29.1/config/default.yml
configuration from /Users/anton/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/rubocop-rspec-1.29.1/config/default.yml
Default configuration from /Users/anton/code/rubocop/config/default.yml
Inheriting configuration from /Users/anton/code/rubocop/.rubocop_todo.yml
Inspecting 1 file
Scanning /Users/anton/code/rubocop/users.rb
C

Offenses:

users.rb:6:5: C: Layout/ClosingParenthesisIndentation: Indent ) to column 5 (not 4)
    )
    ^

1 file inspected, 1 offense detected
Finished in 0.5669199999974808 seconds
```
@antonzaytsev antonzaytsev force-pushed the fix/layout-closing-parenthesis-indentation-multiline-param branch from 8ff6db2 to 1653112 Compare December 17, 2018 07:25
@Drenmi Drenmi merged commit 95e58a2 into rubocop:master Jan 22, 2019
@Drenmi
Copy link
Collaborator

Drenmi commented Jan 22, 2019

Thank you, @antonzaytsev! 🙇

@antonzaytsev antonzaytsev deleted the fix/layout-closing-parenthesis-indentation-multiline-param branch January 22, 2019 07:45
@Drenmi Drenmi mentioned this pull request Feb 10, 2019
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

5 participants