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

Improve ExtraSpacing Cop #7211

Merged
merged 5 commits into from Jul 18, 2019
Merged

Improve ExtraSpacing Cop #7211

merged 5 commits into from Jul 18, 2019

Conversation

jfelchner
Copy link
Contributor

@jfelchner jfelchner commented Jul 16, 2019

A continuation of #5696

This allows for more complex = alignments which previously either caused false positives or autocorrected to the wrong result, or both.

The ultimate result is to be able to give Rubocop something like this:

def batch
  @areas = params[:param].map do
               var_1 = 123_456
               variable_2 = 456_123
           end
  @another = params[:param].map do
               char_1 = begin
                          variable_1_1     = 'a'
                          variable_1_20  = 'b'

                          variable_1_300    = 'c'
                          # A Comment
                          variable_1_4000      = 'd'

                          variable_1_50000     = 'e'
                          puts 'a non-assignment statement without a blank line'
                          some_other_length_variable     = 'f'
                        end
               var_2 = 456_123
             end

  render json: @areas
end

and have it autocorrect it to:

def batch
  @areas   = params[:param].map do
                 var_1      = 123_456
                 variable_2 = 456_123
             end
  @another = params[:param].map do
               char_1 = begin
                          variable_1_1  = 'a'
                          variable_1_20 = 'b'

                          variable_1_300  = 'c'
                          # A Comment
                          variable_1_4000 = 'd'

                          variable_1_50000           = 'e'
                          puts 'a non-assignment statement without a blank line'
                          some_other_length_variable = 'f'
                        end
               var_2  = 456_123
             end

  render json: @areas
end

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.

CHANGELOG.md Outdated Show resolved Hide resolved
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 17, 2019

The changes look reasonable to me, but I'll leave it to @jonas054 to decide whether to accept them.

Copy link
Collaborator

@jonas054 jonas054 left a comment

Choose a reason for hiding this comment

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

I only had one comment. Fix that and we should be ready to merge. 👍

jfelchner and others added 5 commits July 18, 2019 15:33
This allows for more complex `=` alignments which previously either
caused false positives or autocorrected to the wrong result, or both.

The ultimate result is to be able to give Rubocop something like this:

```ruby
def batch
  @areas = params[:param].map do
               var_1 = 123_456
               variable_2 = 456_123
           end
  @another = params[:param].map do
               char_1 = begin
                          variable_1_1     = 'a'
                          variable_1_20  = 'b'

                          variable_1_300    = 'c'
                          # A Comment
                          variable_1_4000      = 'd'

                          variable_1_50000     = 'e'
                          puts 'a non-assignment statement without a blank line'
                          some_other_length_variable     = 'f'
                        end
               var_2 = 456_123
             end

  render json: @areas
end
```

and have it autocorrect it to:

```ruby
def batch
  @areas   = params[:param].map do
                 var_1      = 123_456
                 variable_2 = 456_123
             end
  @another = params[:param].map do
               char_1 = begin
                          variable_1_1  = 'a'
                          variable_1_20 = 'b'

                          variable_1_300  = 'c'
                          # A Comment
                          variable_1_4000 = 'd'

                          variable_1_50000           = 'e'
                          puts 'a non-assignment statement without a blank line'
                          some_other_length_variable = 'f'
                        end
               var_2  = 456_123
             end

  render json: @areas
end
```
Unexpected offenses are now shown in annotaded source format,
making them easier to read.
Why This Change Is Necessary
========================================================================

We want to be able to use this logic with both the ExtraSpacing cop as
well as the SpaceAroundOperators cop.

How These Changes Address the Issue
========================================================================

Added this logic to the PrecedingFollowingAlignment module.

Side Effects Caused By This Change
========================================================================

None known.
Why This Change Is Necessary
========================================================================

We need a way to perform different logic depending on whether the
operator is an assignment or not.

How These Changes Address the Issue
========================================================================

We add a `type` parameter to the `check_operator` method.

Side Effects Caused By This Change
========================================================================

None known.
@jfelchner
Copy link
Contributor Author

@jonas054 rebased!

@bbatsov bbatsov merged commit bd485d6 into rubocop:master Jul 18, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Jul 18, 2019

🚀

@jfelchner
Copy link
Contributor Author

@jonas054 thanks for helping me get this over the line! 1.5 years in the making! 😂

@jonas054
Copy link
Collaborator

@jfelchner No problem, I had fun pairing!

koic added a commit to koic/rails that referenced this pull request Aug 15, 2019
### Summary

RuboCop 0.74.0 has been released.
https://github.com/rubocop-hq/rubocop/releases/tag/v0.74.0

And rubocop-0-74 channel is available in Code Climate.
https://github.com/codeclimate/codeclimate/releases/tag/v0.85.5

This PR specifies the same RuboCop Performance (1.3.0) and RuboCop Rails (2.0.0)
versions as Code Climate's Gemfile.lock.
https://github.com/codeclimate/codeclimate-rubocop/blob/channel/rubocop-0-74/Gemfile.lock#L51-L55

Also, the `EnforcedStyle` of `Layout/IndentationConsistency` has been renamed
from `EnforcedStyle: rails` to `EnforcedStyle: indented_internal_methods`

- rubocop/rubocop#7113
- rubocop/rubocop#7163

And this commit disables `Layout/SpaceAroundOperators`
that was changed from RuboCop 0.74 by rubocop/rubocop#7211.

cf. rails#36943 (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

3 participants