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

Style/MultilineMethodCallIndentation incorrectly autocorrects with nested method calls #3331

Closed
jfelchner opened this issue Jul 20, 2016 · 7 comments

Comments

@jfelchner
Copy link
Contributor

The new indented_relative_to_receiver doesn't work correctly in all instances. It appears that if there is a method call as a parameter to a method, the cop attempts to align with the receiver of the first method instead of align with the receiver of the second.

Expected behavior

expect(MyClass).to have_received(:a_method).
                   with(some: param)

Actual behavior

expect(MyClass).to have_received(:a_method).
with(some: param)

Steps to reproduce the problem

See above

RuboCop version

Include the output of rubocop -V:

$ rubocop -V
0.41.2 (using Parser 2.3.1.2, running on ruby 2.3.0 x86_64-darwin15)
@jonas054
Copy link
Collaborator

First, a point about what we should expect. Since the style is called indented relative to receiver, we can't expect the method name to be aligned with a receiver. I would suggest that the expected behavior (for you, and that may not be what I expect) is

expect(MyClass).to have_received(:a_method).
                     with(some: param)

An extended (contrived) example of what we support today:

# aligned
a = expect(MyClass).to have_received(:a_method).
    with(some: param)

# indented_relative_to_receiver
a = expect(MyClass).to have_received(:a_method).
      with(some: param)

# indented
a = expect(MyClass).to have_received(:a_method).
  with(some: param)

If we're not happy with this, perhaps we should change the aligned style as well to something that makes more sense semantically:

# aligned
a = expect(MyClass).to have_received(:a_method).
                       with(some: param)

# indented_relative_to_receiver
a = expect(MyClass).to have_received(:a_method).
                         with(some: param)

# indented
a = expect(MyClass).to have_received(:a_method).
  with(some: param)

I'm not sure if there are any unwanted implications, going in this direction.

@jfelchner
Copy link
Contributor Author

jfelchner commented Jul 29, 2016

@jonas054 yeah (for me) your example at the bottom of your comment is what I would expect for indented_relative_to_reciever. expect is not the reciever of with, have_received is.

I don't think it's necessary to make a breaking change regarding what aligned does. After all, if I wanted:

a = expect(MyClass).to have_received(:a_method).
                       with(some: param)

Couldn't I just do indented_relative_to_receiver with an IndentationWidth of 0?

@jfelchner
Copy link
Contributor Author

I'm just trying to save you some work. I would assume that there are people who would want this style:

a = expect(MyClass).to have_received(:a_method).
    with(some: param)

@jfelchner
Copy link
Contributor Author

@jonas054 is there something that I can do to help here? I know your time is valuable.

@jonas054
Copy link
Collaborator

@jfelchner Sorry this is taking so long. If you can write some failing test cases and submit them as a gist, a PR, a branch, or whatever, I'll do the implementation.

@jfelchner
Copy link
Contributor Author

@jonas054 I'm on it!

@jfelchner
Copy link
Contributor Author

@jonas054 I have the test done but I'm actually reworking the other ones so that they better reflect what I'm looking for. I haven't forgotten about this. 👍

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

No branches or pull requests

2 participants