Autocorrect BracesAroundHashParameters leaves extra space

[aripollak]

When running rubocop with autocorrect, it changes a file like this:

-      get :index, { q: { 'unknown_key' => 'value' } }
+      get :index,  q: { 'unknown_key' => 'value' }

There should only be one space after the comma instead of two.

[mattjmcnaughton]

The relevant code is here. Right now the braces are removed, but nothing else, which leaves two spaces. A fix could be checking if the character after the first brace is a hash and removing it if so. I'd love to get involved with Rubocop, so if no one objects, I can submit a pull request.

[mattjmcnaughton]

Also I replicated the issue - here's the rubocop -v output.

0.28.0 (using Parser 2.2.0.pre.8, running on ruby 2.0.0 x86_64-darwin13.1.0)

[jonas054]

@mattjmcnaughton Please go ahead with a pull request. Removing the space together with the brace sounds like a good plan. Do it for both braces. There's a method available in all cops called Util#range_with_surrounding_space that you can use.

[mattjmcnaughton]

Ok so I made the changes you recommended @jonas054, and the changes fix @aripollak issue. But, it causes a lot of the other tests to fail. I'll include an output from a test in braces_around_hash_parameter_spec.rb that highlights the issue.

corrected = autocorrect_source(cop, ['where({ x: 1, y: 2, })'])
expect(corrected).to eq('where( x: 1, y: 2, )')

The output of the rspec test is:
expected: "where( x: 1, y: 2 )" got: "where(x: 1, y: 2)

The same thing occurs for a lot of the other tests of the BracesAroundHashParameter cop as the tests expect nothing to change about the amount of whitespace, and with the new updates, all the white space is being removed. This change does not have any effects on tests outside of braces_around_hash_parameter_spec.

To me, removing all the extra white space seems like a good idea. But I'm also new to this project, so I may totally be missing something. Let me know what you think is best and I'll make a pull request accordingly!

[jonas054]

The central auto-correction logic has changed a lot over time, and so have the principles for how to do automatic corrections in the cops. At one point it was really important that corrections were made as small as possible in order to minimize the risk for interference ("clobbering") between cops. This risk is pretty much eliminated now since auto-correction is done one cop at a time, with writing to file and re-parsing the file after each cop has finished.

So the behavior we had was the best choice when it was implemented, but now it's better to include whitespace clean-up. Go ahead and update the expected code in the examples by removing the extra whitespace.

[mattjmcnaughton]

Ok awesome - thanks @jonas054! There is still one test that doesn't pass that I'd love some help with.

It is this one in cli_spec. I'm thinking it make but something to do with the way newlines are handled but I'm not sure.

Here's the diff between expected and actual.

# encoding: utf-8
       -expect(subject[:address]).to eq(
       -  street1:     '1 Market',
       -  street2:     '#200',
       -  city:        'Some Town',
       -  state:       'CA',
       -  postal_code: '99999-1111'
       +expect(subject[:address]).to eq(street1:     '1 Market',
       +                                street2:     '#200',
       +                                city:        'Some Town',
       +                                state:       'CA',
       +                                postal_code: '99999-1111'
        )

Also, does the expected output for this test look right? Frankly I'm not sure what is being tested and what the expected behavior should be. Right now it passes, but I don't know if the output is what we actually want.

Let me know if it's easier for me to make a pull request now - thought I'd wait until all the tests were all passing and I had a chance to update the change logs but I can do it sooner if that's easier.

[jonas054]

I hadn't examined Util#range_with_surrounding_space in detail before I recommended it to you, and it turns out that it doesn't treat newlines in a uniform way. It will count one newline on the right hand side of the given range as "surrounding space", but that's it. So inside }, one newline will be removed, but that won't happen inside }. I'm not sure if you should change it or not. You can try and see what happens in other spec examples. Anyway, this is the reason for the output from cli_spec.rb.

The other spec example that you link to looks as expected.

[mattjmcnaughton]

Ok, so as per my understanding of the original Util#range_with_surrounding_space method, right now if there is a newline within }, it will be removed. However, if there is a newline within {, it will not be removed. This is based on the current code seen below.

def range_with_surrounding_space(range, side = :both)
    src = @processed_source.buffer.source
    go_left = side == :left || side == :both
    go_right = side == :right || side == :both
    begin_pos = range.begin_pos
    begin_pos -= 1 while go_left && src[begin_pos - 1] =~ /[ \t]/
    end_pos = range.end_pos
    end_pos += 1 while go_right && src[end_pos] =~ /[ \t]/
    end_pos += 1 if go_right && src[end_pos] == "\n"
    Parser::Source::Range.new(@processed_source.buffer, begin_pos, end_pos)
end

I think there are two options.

First

Adding code to remove newlines within { makes the behavior uniform, but does not fix the cli_spec.rb error. Basically now the begin_pos part of the above method now looks like this.

begin_pos = range.begin_pos
begin_pos -= 1 while go_left && src[begin_pos - 1] =~ /[ \t]/
begin_pos -= 1 if go_left && src[begin_pos - 1] == "\n"

This option makes the most sense to me because it gives a uniform behavior to Util#range_with_surrounding_space. It would require the cli_spec.rb test that is failing to be rewritten.

Second

Remove the end_pos += 1 if go_right && src[end_pos] == "\n" so that no new lines are removed. This fixes the cli_spec.rb error, but it introduces two new errors in space_around_operators_spec.rb here and here.

To me, option 1 makes the most sense, but its up to you!

[jonas054]

I agree. Option 1 looks better.