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

False Positive: Layout/ExtraSpacing #5265

Closed
jfelchner opened this issue Dec 15, 2017 · 7 comments
Closed

False Positive: Layout/ExtraSpacing #5265

jfelchner opened this issue Dec 15, 2017 · 7 comments
Labels

Comments

@jfelchner
Copy link
Contributor

jfelchner commented Dec 15, 2017

Reproducible Case

# baz.rb
class Foo
  def batch
    @areas = params[:param].map do |ca_params|
               ca_params = ActionController::Parameters.new(stuff)
               event     = Event.find(stuff[:event_id])

               whatever
             end

    render json: @areas
  end
end
# .rubocop.yml
Layout/ExtraSpacing:
  Enabled:                                    true
  ForceEqualSignAlignment:                    true
> rubocop --only='Layout/ExtraSpacing' baz.rb

Layout/ExtraSpacing throws errors on lines 3 and 4 which is incorrect.

History

As I mentioned in this comment back in the day, when checking if there is extra spacing, it should only look at a previous line of the same indentation level. There are three rules which need added to this cop:

The Three Rules

  1. If the previous line has greater indentation, disregard this line and instead analyze the one prior to it. Continue doing this until either Rule 2 or Rule 3 applies.
  2. If the previous line has the same indentation, stop and verify. You're done.
  3. If the previous line has less indentation, stop. There is nothing to verify the line against. You're done.

Let's look at an example with these three rules:

A Properly-Aligned Example

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

      render json: @areas
    end
  • var_2 finds the last line with an indentation equal to it skipping lines which are more indented than it is
  • variable_1_2 finds the line with an indentation equal to it and stops because the previous line is
  • variable_1_1 finds the last line with an indentation equal to it but stops because the previous line is indented less than it
  • char_1 finds the last line with an indentation equal to it but stops because the previous line is indented less than it
  • @another finds the last line with an indentation equal to it skipping lines which are more indented than it is

etc, etc.

In order to get a "correct" ExtraSpacing cop, these rules need to be added.

@neontapir
Copy link
Contributor

I tried running the sample method against rubocop 0.52 and wasn't able to reproduce the issue.

@jfelchner
Copy link
Contributor Author

@neontapir This has been busted for over a year, but it looks like some changes were made to ExtraSpacing in the last few days. I don't think the changes solve my issue, but I'll come back and give an offending example if it's still broken. Closing for now.

@jfelchner jfelchner reopened this Dec 29, 2017
@jfelchner
Copy link
Contributor Author

@neontapir I'm assuming you didn't have ForceEqualSignAlignment set to true in your configuration.

@jfelchner
Copy link
Contributor Author

I've updated the original issue to describe the exact reproducible use case.

@jfelchner jfelchner changed the title Bug In Layout/ExtraSpacing Cop Causes False Positives False Positive: Layout/ExtraSpacing Dec 29, 2017
@bbatsov bbatsov added the bug label Sep 23, 2018
@stale
Copy link

stale bot commented May 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label May 8, 2019
@jfelchner
Copy link
Contributor Author

This is still an issue.

@stale stale bot removed the stale Issues that haven't been active in a while label May 10, 2019
@jfelchner
Copy link
Contributor Author

Fixed in #7211

bbatsov pushed a commit that referenced this issue Jul 18, 2019
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
```
tas50 added a commit to chef/cookstyle that referenced this issue Oct 30, 2019
This resolves a large number of warnings and gives us a new cop for
automatically migrating the namespace of the cop disables in comments

* [#7407](rubocop/rubocop#7407): Make `Style/FormatStringToken` work inside hashes. ([@buehmann][])
* [#7389](rubocop/rubocop#7389): Fix an issue where passing a formatter might result in an error depending on what character it started with. ([@jfhinchcliffe][])
* [#7397](rubocop/rubocop#7397): Fix extra comments being added to the correction of `Style/SafeNavigation`. ([@rrosenblum][])
* [#7378](rubocop/rubocop#7378): Fix heredoc edge cases in `Layout/EmptyLineAfterGuardClause`. ([@gsamokovarov][])
* [#7404](rubocop/rubocop#7404): Fix a false negative for `Layout/IndentAssignment` when multiple assignment with line breaks on each line. ([@koic][])
* [#7256](rubocop/rubocop#7256): Fix an error of `Style/RedundantParentheses` on method calls where the first argument begins with a hash literal. ([@halfwhole][])
* [#7263](rubocop/rubocop#7263): Make `Layout/SpaceInsideArrayLiteralBrackets` properly handle tab-indented arrays. ([@buehmann][])
* [#7252](rubocop/rubocop#7252): Prevent infinite loops by making `Layout/SpaceInsideStringInterpolation` skip over interpolations that start or end with a line break. ([@buehmann][])
* [#7262](rubocop/rubocop#7262): `Lint/FormatParameterMismatch` did not recognize named format sequences like `%.2<name>f` where the name appears after some modifiers. ([@buehmann][])
* [#7253](rubocop/rubocop#7253): Fix an error for `Lint/NumberConversion` when `#to_i` called without a receiver. ([@koic][])
* [#7271](rubocop/rubocop#7271), [#6498](rubocop/rubocop#6498): Fix an interference between `Style/TrailingCommaIn*Literal` and `Layout/Multiline*BraceLayout` for arrays and hashes. ([@buehmann][])
* [#7241](rubocop/rubocop#7241): Make `Style/FrozenStringLiteralComment` match only true & false. ([@tejasbubane][])
* [#7290](rubocop/rubocop#7290): Handle inner conditional inside `else` in `Style/ConditionalAssignment`. ([@jonas054][])
* [#5788](rubocop/rubocop#5788): Allow block arguments on separate lines if line would be too long in `Layout/MultilineBlockLayout`. ([@jonas054][])
* [#7305](rubocop/rubocop#7305): Register `Style/BlockDelimiters` offense when block result is assigned to an attribute. ([@mvz][])
* [#4802](rubocop/rubocop#4802): Don't leave any `Lint/UnneededCopEnableDirective` offenses undetected/uncorrected. ([@jonas054][])
* [#7326](rubocop/rubocop#7326): Fix a false positive for `Style/AccessModifierDeclarations` when access modifier name is used for hash literal value. ([@koic][])
* [#3591](rubocop/rubocop#3591): Handle modifier `if`/`unless` correctly in `Lint/UselessAssignment`. ([@jonas054][])
* [#7161](rubocop/rubocop#7161): Fix `Style/SafeNavigation` cop for preserve comments inside if expression. ([@tejasbubane][])
* [#5212](rubocop/rubocop#5212): Avoid false positive for braces that are needed to preserve semantics in `Style/BracesAroundHashParameters`. ([@jonas054][])
* [#7353](rubocop/rubocop#7353): Fix a false positive for `Style/RedundantSelf` when receiver and multiple assigned lvalue have the same name. ([@koic][])
* [#7353](rubocop/rubocop#7353): Fix a false positive for `Style/RedundantSelf` when a self receiver is used as a method argument. ([@koic][])
* [#7358](rubocop/rubocop#7358): Fix an incorrect autocorrect for `Style/NestedModifier` when parentheses are required in method arguments. ([@koic][])
* [#7361](rubocop/rubocop#7361): Fix a false positive for `Style/TernaryParentheses` when only the closing parenthesis is used in the last line of condition. ([@koic][])
* [#7369](rubocop/rubocop#7369): Fix an infinite loop error for `Layout/IndentAssignment` with `Layout/IndentFirstArgument` when using multiple assignment. ([@koic][])
* [#7177](rubocop/rubocop#7177), [#7370](rubocop/rubocop#7370): When correcting alignment, do not insert spaces into string literals. ([@buehmann][])
* [#7367](rubocop/rubocop#7367): Fix an error for `Style/OrAssignment` cop when `then` branch body is empty. ([@koic][])
* [#7363](rubocop/rubocop#7363): Fix an incorrect autocorrect for `Layout/SpaceInsideBlockBraces` and `Style/BlockDelimiters` when using multiline empty braces. ([@koic][])
* [#7212](rubocop/rubocop#7212): Fix a false positive for `Layout/EmptyLinesAroundAccessModifier` and `UselessAccessModifier` when using method with the same name as access modifier around a method definition. ([@koic][])
* [#7217](rubocop/rubocop#7217): Make `Style/TrailingMethodEndStatement` work on more than the first `def`. ([@buehmann][])
* [#7190](rubocop/rubocop#7190): Support lower case drive letters on Windows. ([@jonas054][])
* Fix the auto-correction of `Lint/UnneededSplatExpansion` when the splat expansion of `Array.new` with a block is assigned to a variable. ([@rrosenblum][])
* [#5628](rubocop/rubocop#5628): Fix an error of `Layout/SpaceInsideStringInterpolation` on interpolations with multiple statements. ([@buehmann][])
* [#7128](rubocop/rubocop#7128): Make `Metrics/LineLength` aware of shebang. ([@koic][])
* [#6861](rubocop/rubocop#6861): Fix a false positive for `Layout/IndentationWidth` when using `EnforcedStyle: outdent` of `Layout/AccessModifierIndentation`. ([@koic][])
* [#7235](rubocop/rubocop#7235): Fix an error where `Style/ConditionalAssignment` would swallow a nested `if` condition. ([@buehmann][])
* [#7242](rubocop/rubocop#7242): Make `Style/ConstantVisibility` work on non-trivial class and module bodies. ([@buehmann][])
* [#5265](rubocop/rubocop#5265): Improved `Layout/ExtraSpacing` cop to handle nested consecutive assignments. ([@jfelchner][])
* [#7215](rubocop/rubocop#7215): Make it clear what's wrong in the message from `Style/GuardClause`. ([@jonas054][])
* [#7245](rubocop/rubocop#7245): Make cops detect string interpolations in more contexts: inside of backticks, regular expressions, and symbols. ([@buehmann][])
 [#7275](rubocop/rubocop#7275): Make `Style/VariableName` aware argument names when invoking a method. ([@koic][])
* [#3534](rubocop/rubocop#3534): Make `Style/IfUnlessModifier` report and auto-correct modifier lines that are too long. ([@jonas054][])
* [#7261](rubocop/rubocop#7261): `Style/FrozenStringLiteralComment` no longer inserts an empty line after the comment. This is left to `Layout/EmptyLineAfterMagicComment`. ([@buehmann][])
* [#7091](rubocop/rubocop#7091): `Style/FormatStringToken` now detects format sequences with flags and modifiers. ([@buehmann][])
* [#7319](rubocop/rubocop#7319): Rename `IgnoredMethodPatterns` option to `IgnoredPatterns` option for `Style/MethodCallWithArgsParentheses`. ([@koic][])
* [#7345](rubocop/rubocop#7345): Mark unsafe for `Style/YodaCondition`. ([@koic][])
* [#7170](rubocop/rubocop#7170): Fix a false positive for `Layout/RescueEnsureAlignment` when def line is preceded with `private_class_method`. ([@tatsuyafw][])
* [#7186](rubocop/rubocop#7186): Fix a false positive for `Style/MixinUsage` when using inside multiline block and `if` condition is after `include`. ([@koic][])
* [#7099](rubocop/rubocop#7099): Fix an error of `Layout/RescueEnsureAlignment` on assigned blocks. ([@tatsuyafw][])
* [#5088](rubocop/rubocop#5088): Fix an error of `Layout/MultilineMethodCallIndentation` on method chains inside an argument. ([@buehmann][])
* [#4719](rubocop/rubocop#4719): Make `Layout/Tab` detect tabs between string literals. ([@buehmann][])
* [#7203](rubocop/rubocop#7203): Fix an infinite loop error for `Layout/SpaceInsideBlockBraces` when `EnforcedStyle: no_space` with `SpaceBeforeBlockParameters: false` are set in multiline block. ([@koic][])
* [#6653](rubocop/rubocop#6653): Fix a bug where `Layout/IndentHeredoc` would remove empty lines when autocorrecting heredocs. ([@buehmann][])
* [#7188](rubocop/rubocop#7188): Include inspected file location in auto-correction error. ([@pocke][])

Signed-off-by: Tim Smith <tsmith@chef.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants