ExtraSpacing Double Checks

[jfelchner]

@jonas054 I wanted to throw out a few ExtraSpacing offenses and possible false negatives and make sure the cop is working as intended.

Before I get started, I just wanted to say that this cop is 10,000 times better now and caught a bunch of errors. As it stands, in our project, we still can't currently enable it by default, but we can run it selectively to catch the correct misspacings.

Alignment with Previous Line

In the majority of cases, the current behavior is what I would want. So that if you had:

something = 'something else'

another   = 'another thing'

it would consider those to not have any extra spacing.

However, it causes a wrinkle with something like:

    base.class_eval do
      index_name    "index name"
      document_type name

      after_commit  on: [:create] do
        Workers::IndexUpdate.perform_async(:index, 'Thingy', id)
      end

      after_commit  on: [:update] do
        Workers::IndexUpdate.perform_async(:index, 'Thingy', id)
      end
    end

In this example, the extra spacing before the on in the :update line is caught, but the space before the on on the :create line is not because (I'm assuming) it's looking at how name is aligned above it.

Alignment with Method Calls on the Previous Line

    self.method_call_one_with_a_really_long_name   = config(something,
                                                            'whatever',
                                                            another_thing).
                                                     to_f
    self.method_call_two                           = config(something,
                                                            'blah',
                                                            yet_another_thing).
                                                     to_f
    self.method_call_three                         = config(something,
                                                            'nice_going',
                                                            rubocop_rules).
                                                     to_f

In this case I want to keep all the method calls lined up but ExtraSpacing wants to collapse the first lines.

    self.method_call_one_with_a_really_long_name = config(something,
                                                            'whatever',
                                                            another_thing).
                                                     to_f
    self.method_call_two = config(something,
                                                            'blah',
                                                            yet_another_thing).
                                                     to_f
    self.method_call_three = config(something,
                                                            'nice_going',
                                                            rubocop_rules).
                                                     to_f

Multiline Alignment

A lot of the alignment I do is that which spans multiple lines. It makes the code much easier to scan.

Currently when ExtraSpacing sees the following code:

    first_product          = FactoryGirl.create(:product,
                                                :with_trait)
    second_product         = FactoryGirl.create(:product,
                                                base_model_id: first_product.base_model_id)
    third_product          = FactoryGirl.create(:product,
                                                :with_trait)
    color_attribute_name   = FactoryGirl.create(:attribute,
                                                name:   'Color',
                                                values: %w{red})
    red_value              = color_attribute_name.values.first
    size_attribute_name    = FactoryGirl.create(:attribute,
                                                name:   'Size',
                                                values: %w{small large})
    carrier_attribute_name = FactoryGirl.create(:attribute,
                                                name:   'Carrier',
                                                values: %w{verizon})

it wants to turn it into this:

    first_product = FactoryGirl.create(:product,
                                       :with_trait)
    second_product = FactoryGirl.create(:product,
                                        base_model_id: first_product.base_model_id)
    third_product = FactoryGirl.create(:product,
                                       :with_trait)
    color_attribute_name = FactoryGirl.create(:attribute,
                                              name:   'Color',
                                              values: %w{red})
    red_value            = color_attribute_name.values.first
    size_attribute_name  = FactoryGirl.create(:attribute,
                                              name:   'Size',
                                              values: %w{small large})
    carrier_attribute_name = FactoryGirl.create(:attribute,
                                                name:   'Carrier',
                                                values: %w{verizon})

Alignment of Hash Values

Currently it appears that ExtraSpacing conflicts (possibly?) with the AlignHash cop's autocorrection. It seems as though ExtraSpacing is trying to correct spacing on hash values which I believe it should be prohibited from doing since that's the job of the AlignHash cop.

When I attempt to run it, I get:

/usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:126:in `check_for_infinite_loop': Infinite loop detected in /Users/jfelchner/Projects/my_project/my_file.rb. (RuboCop::Runner::InfiniteCorrectionLoop)
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:101:in `block in do_inspection_loop'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:100:in `loop'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:100:in `do_inspection_loop'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:78:in `process_file'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:53:in `block in inspect_files'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:51:in `each'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:51:in `inspect_files'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/runner.rb:30:in `run'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/lib/rubocop/cli.rb:26:in `run'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/bin/rubocop:13:in `block in <top (required)>'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/2.2.0/benchmark.rb:303:in `realtime'
        from /usr/local/var/rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/rubocop-0.33.0/bin/rubocop:12:in `<top (required)>'
        from /usr/local/var/rbenv/versions/2.2.2/bin/rubocop:23:in `load'
        from /usr/local/var/rbenv/versions/2.2.2/bin/rubocop:23:in `<main>'

Proposed Solution

So even though it looks like a lot, I think that the solution is actually pretty simple. I think that, when ExtraSpacing is trying to determine whether alignment is being attempted, rather than looking on the previous line, it needs to look at the previous line with the same indentation as the current line.

If it does that, I think it will solve all of my issues (with the exception of the first one which is not that big of a deal IMO, but I wanted to make you aware).

Thank You

Thanks for making this cop so awesome @jonas054. It caught a ton of errors in our code. 😀

[jonas054]

@jfelchner Thanks for the detailed description. The infinite correction loop is of course a bug. Your suggested correction for the other false positive problems sounds reasonable. I'll try it out. The false negative is less of a problem, I agree.

[jfelchner]

@jonas054 would you like me to make a separate issue for the bug?

[jonas054]

@jfelchner Yes, that would be good. Please include an example of a hash literal that triggers the bug.

[hackling]

Deleted my wall of text - I'm an idiot. AllowForAlignment is what I was looking for.