Skip to content

Commit

Permalink
[Fix #3331] MultilineMethodCallIndentation references only method chain
Browse files Browse the repository at this point in the history 
* Change alignment_base to a more readable case statement
* Add CHANGELOG entry
* Update documentation with indented_relative_to_receiver
* Move some indentation tests out of the 'common' shared example

  We're going to change the behavior of `indented_relative_to_receiver`
  such that it will not behave the same as `aligned` and `indented` any
  longer.

  This keeps the specs passing.

* Fix MultilineMethodCallIndentation references only method chain

  It appears as if the current behavior was potentially desired by the
  original author, however I feel as if it's incorrect.

  Not only does `aligned` and `indented` already behave the same as the
  current behavior of `indented_relative_to_receiver`, but the name of
  `indented_relative_to_receiver` is completely misleading to what the
  current behavior actually does.

  The current code looks all the way up the call stack such that if it's
  an _argument_ to a method call, it traverses up _that_ method's call
  chain until it can go no further.

  Take this code as an example:

  ```ruby
  existing_customer = Customer.find(customer_id)
  order             = existing_customer
  .orders
  .status(SUCCESSFUL_STATES)
  .order(:scheduled_delivery_date)
  ```

  Whereas before it would have wanted this:

  ```ruby
  existing_customer = Customer.find(customer_id)
  order             = existing_customer
    .orders
    .status(SUCCESSFUL_STATES)
    .order(:scheduled_delivery_date)
  ```

  Now it wants this:

  ```ruby
  existing_customer = Customer.find(customer_id)
  order             = existing_customer
                        .orders
                        .status(SUCCESSFUL_STATES)
                        .order(:scheduled_delivery_date)
  ```

  Or this:

  ```ruby
  expect(my_reading).to be.within(.005)
  .of(5.0)
  ```

  Whereas before it would have wanted this:

  ```ruby
  expect(my_reading).to be.within(.005)
    .of(5.0)
  ```

  Now it wants this:

  ```ruby
  expect(my_reading).to be.within(.005)
                          .of(5.0)
  ```

  if you want to always indent relative to the leftmost character or
  column 0, then this cop _already_ has a style for those scenarios:
  `aligned` and `indented`.
  • Loading branch information
@jfelchner @bbatsov
jfelchner authored and bbatsov committed Mar 23, 2017
1 parent f76fc44 commit 586944d
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 64 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
* [#4055](https://github.com/bbatsov/rubocop/pull/4055): Add parameters count to offense message for `Metrics/ParameterLists` cop. ([@pocke][])
* [#4081](https://github.com/bbatsov/rubocop/pull/4081): Allow `Marshal.load` if argument is a `Marshal.dump` in `Security/MarshalLoad` cop. ([@droptheplot][])
* [#4124](https://github.com/bbatsov/rubocop/issues/4124): Make `Style/SymbolArray` cop to enable by default. ([@pocke][])
* [#3331](https://github.com/bbatsov/rubocop/issues/3331): Change `Style/MultilineMethodCallIndentation` `indented_relative_to_receiver` to indent relative to the reciever and not relative to the caller. ([@jfelchner][])

### Bug fixes

Expand Down
57 changes: 38 additions & 19 deletions lib/rubocop/cop/style/multiline_method_call_indentation.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,45 @@ module Style
#
# @example
# # bad
# while a
# while myvariable
# .b
# something
# # do something
# end
#
# # good, EnforcedStyle: aligned
# while a
# while myvariable
# .b
# something
# # do something
# end
#
# # good, EnforcedStyle: aligned
# Thing.a
# .b
# .c
#
# # good, EnforcedStyle: indented
# while a
# .b
# something
# # good, EnforcedStyle: indented,
# IndentationWidth: 2
# while myvariable
# .b
#
# # do something
# end
#
# # good, EnforcedStyle: indented_relative_to_receiver,
# IndentationWidth: 2
# while myvariable
# .a
# .b
#
# # do something
# end
#
# # good, EnforcedStyle: indented_relative_to_receiver,
# IndentationWidth: 2
# myvariable = Thing
# .a
# .b
# .c
class MultilineMethodCallIndentation < Cop
include ConfigurableEnforcedStyle
include AutocorrectAlignment
Expand Down Expand Up @@ -112,15 +130,15 @@ def no_base_message(lhs, rhs, node)
end

def alignment_base(node, rhs, given_style)
return nil if given_style == :indented

if given_style == :indented_relative_to_receiver
receiver_base = receiver_alignment_base(node)
return receiver_base if receiver_base
case given_style
when :aligned
semantic_alignment_base(node, rhs) ||
syntactic_alignment_base(node, rhs)
when :indented
nil
when :indented_relative_to_receiver
receiver_alignment_base(node)
end

semantic_alignment_base(node, rhs) ||
syntactic_alignment_base(node, rhs)
end

def syntactic_alignment_base(lhs, rhs)
Expand Down Expand Up @@ -162,10 +180,11 @@ def semantic_alignment_base(node, rhs)
# .b
# .c
def receiver_alignment_base(node)
node = semantic_alignment_node(node)
return unless node
node = node.receiver while node.receiver
node = node.parent
node = node.parent until node.loc.dot

node.receiver.source_range
node.receiver.source_range if node
end

def semantic_alignment_node(node)
Expand Down
34 changes: 26 additions & 8 deletions manual/cops_style.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3484,27 +3484,45 @@ that span more than one line.

```ruby
# bad
while a
while myvariable
.b
something
# do something
end

# good, EnforcedStyle: aligned
while a
while myvariable
.b
something
# do something
end

# good, EnforcedStyle: aligned
Thing.a
.b
.c

# good, EnforcedStyle: indented
while a
.b
something
# good, EnforcedStyle: indented,
IndentationWidth: 2
while myvariable
.b

# do something
end

# good, EnforcedStyle: indented_relative_to_receiver,
IndentationWidth: 2
while myvariable
.a
.b

# do something
end

# good, EnforcedStyle: indented_relative_to_receiver,
IndentationWidth: 2
myvariable = Thing
.a
.b
.c
```

### Important attributes
Expand Down
116 changes: 79 additions & 37 deletions spec/rubocop/cop/style/multiline_method_call_indentation_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,6 @@
expect(cop.messages).to be_empty
end

it 'registers an offense for one space indentation of second line' do
inspect_source(cop,
['a',
' .b'])
expect(cop.messages).to eq(['Use 2 (not 1) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['.b'])
end

it 'registers an offense for proc call without a selector' do
inspect_source(cop,
['a',
' .(args)'])
expect(cop.messages).to eq(['Use 2 (not 1) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['.('])
end

it 'accepts no extra indentation of third line' do
inspect_source(cop,
[' a.',
Expand Down Expand Up @@ -100,14 +82,6 @@
expect(cop.messages).to be_empty
end

it 'accepts even indentation of consecutive lines in typical RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to be_empty
end

it 'accepts any indentation of parameters to #[]' do
inspect_source(cop,
['payment = Models::IncomingPayments[',
Expand All @@ -116,17 +90,6 @@
expect(cop.messages).to be_empty
end

it 'registers an offense for extra indentation of 3rd line in typical ' \
'RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to eq(['Use 2 (not 6) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['from'])
end

it "doesn't fail on unary operators" do
inspect_source(cop,
['def foo',
Expand All @@ -138,6 +101,14 @@
end

shared_examples 'common for aligned and indented' do
it 'accepts even indentation of consecutive lines in typical RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to be_empty
end

it 'registers an offense for no indentation of second line' do
inspect_source(cop,
['a.',
Expand Down Expand Up @@ -183,6 +154,35 @@
'multiple lines.'])
expect(cop.highlights).to eq(['b'])
end

it 'registers an offense for extra indentation of 3rd line in typical ' \
'RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to eq(['Use 2 (not 6) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['from'])
end

it 'registers an offense for proc call without a selector' do
inspect_source(cop,
['a',
' .(args)'])
expect(cop.messages).to eq(['Use 2 (not 1) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['.('])
end

it 'registers an offense for one space indentation of second line' do
inspect_source(cop,
['a',
' .b'])
expect(cop.messages).to eq(['Use 2 (not 1) spaces for indenting an ' \
'expression spanning multiple lines.'])
expect(cop.highlights).to eq(['.b'])
end
end

context 'when EnforcedStyle is aligned' do
Expand Down Expand Up @@ -531,6 +531,19 @@
expect(cop.offenses).to be_empty
end

it 'accepts indentation of consecutive lines in typical RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to be_empty

inspect_source(cop,
['expect { Foo.new }.to change { Bar.count }',
' .from(1).to(2)'])
expect(cop.messages).to be_empty
end

it 'registers an offense for no indentation of second line' do
inspect_source(cop,
['a.',
Expand All @@ -540,6 +553,35 @@
expect(cop.highlights).to eq(['b'])
end

it 'registers an offense for extra indentation of 3rd line in typical ' \
'RSpec code' do
inspect_source(cop,
['expect { Foo.new }.',
' to change { Bar.count }.',
' from(1).to(2)'])
expect(cop.messages).to eq(['Indent `from` 2 spaces more than `change ' \
'{ Bar.count }` on line 2.'])
expect(cop.highlights).to eq(['from'])
end

it 'registers an offense for proc call without a selector' do
inspect_source(cop,
['a',
' .(args)'])
expect(cop.messages).to eq(['Indent `.(` 2 spaces more than `a` on ' \
'line 1.'])
expect(cop.highlights).to eq(['.('])
end

it 'registers an offense for one space indentation of second line' do
inspect_source(cop,
['a',
' .b'])
expect(cop.messages).to eq(['Indent `.b` 2 spaces more than `a` on ' \
'line 1.'])
expect(cop.highlights).to eq(['.b'])
end

it 'registers an offense for 3 spaces indentation of second line' do
inspect_source(cop,
['a.',
Expand Down

0 comments on commit 586944d

Please sign in to comment.