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/SpaceAroundOperators #8906

Merged

Conversation

koic
Copy link
Member

@koic koic commented Oct 19, 2020

This PR fixes a false positive for Layout/SpaceAroundOperators when upward alignment. Two false positive examples are shown.

  1. @output and @logger assignments are aligned with the same margin
@integer_message = 12345
@output  = StringIO.new
@logger  = Logger.new(@output)
  1. expected and tagging assignments are aligned with a blank line in between
expected = posts(:welcome)

tagging  = Tagging.all.merge!(includes: :taggable).find(taggings(:welcome_general).id)
assert_no_queries { assert_equal expected, tagging.taggable }

I got this issue feedback from @kamipo. Thank you!

cf: rails/rails#36943


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

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 19, 2020

I think the docs should be expanded to cover this. In particular I'm not quite sure that alignment should stretch over blank lines, as they are typically used to separate logical blocks, although on the other hand it's hard to imagine there are two logical blocks if the end of the previous and the begining of the next block are assignments. Tricky case, but I'm willing to accept this change if we explain the behaviour clearly.

@koic koic force-pushed the fix_false_positive_for_space_around_operators branch from 0004add to bd8d15f Compare October 20, 2020 18:57
@koic
Copy link
Member Author

koic commented Oct 20, 2020

Thank you for your feedback. I've added the doc for this change.
Let me add a little explanation. This was a potential (breaking) change vertical alignment consisting of one or more whitespace around operators at #7211, for part of users it was an unexpected change.
e.g. rails/rails#36943 (comment)

On the other hand, I agree that there are cases where #7211 behavior is preferred rather than vertical alignment. So, It may be better to make it configurable in future.

This PR fixes a false positive for `Layout/SpaceAroundOperators`
when upward alignment. Two false positive examples are shown.

1. `output` and `logger` assignments are aligned with the same margin

```ruby
integer_message = 12345
output  = StringIO.new
logger  = Logger.new(output)
```

2. `expected` and `tagging` assignments are aligned with a blank line in between

```ruby
expected = posts(:welcome)

tagging  = Tagging.all.merge!(includes: :taggable).find(taggings(:welcome_general).id)
assert_no_queries { assert_equal expected, tagging.taggable }
```

cf: rails/rails#36943
@koic koic force-pushed the fix_false_positive_for_space_around_operators branch from bd8d15f to 9b00c9a Compare October 20, 2020 19:04
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 21, 2020

We should also add good examples with vertical alignment but this can be done later. Thanks for working on this!

@bbatsov bbatsov merged commit 4673957 into rubocop:master Oct 21, 2020
@koic koic deleted the fix_false_positive_for_space_around_operators branch October 21, 2020 06:57
kamipo added a commit to kamipo/rails that referenced this pull request Oct 23, 2020
We prefer space around operators, but `Layout/SpaceAroundOperators` cop
was temporarily disabled in rails#36943 since that cop changed to check
alignment strictly somehow.

In RuboCop 1.0.0, that is fixed by rubocop/rubocop#8906.

Related rails#38034 (comment),
rails#39770 (comment).
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