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

Enforce one parameter per line when method parameters span more than one line #5184

Closed
deivid-rodriguez opened this issue Dec 4, 2017 · 14 comments

Comments

@deivid-rodriguez
Copy link
Contributor

Not sure if this should be considered a bug in the Layout/AlignParameters cop, a new style for it, or a separate cop, but I'd certainly like this to be signaled as a style offense.

Given the following code

a(1,
  2, 3,
  4)

Expected behavior

I would expect rubocop to tell me

Layout/AlignParameters: Align the parameters of a method call if they span more than one line.

for the third parameter, since it's not aligned with the rest of them.

Actual behavior

No offenses are registered.

Steps to reproduce the problem

$ echo "
> a(1,
>   2, 3,
>   4)" | bundle exec rubocop --stdin hola.rb
Inspecting 1 file
.

1 file inspected, no offenses detected

RuboCop version

$ rubocop -V
0.51.0 (using Parser 2.4.0.2, running on ruby 2.4.2 x86_64-linux)
@mikegee
Copy link
Contributor

mikegee commented Dec 4, 2017

Sometimes when the method invocations are long I'll write it like:

long_method_name_here_maybe(first_of_too_many_arguments, a_second_argument_here,
                            third_argument_goes_down_here, maybe_a_forth_too)

I'm fine with such a cop existing, but I'm not sure if I want it flagging code like mine by default.

@deivid-rodriguez
Copy link
Contributor Author

Yeah, I'm totally fine with this not being enabled by default, although reading Layout/AlignParameters error message, this might also be considered a bug.

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 7, 2017

It's definitely not a bug. What you describe makes sense, but is not really related to this cop. Perhaps we can have a different cop that enforces an uniform number of params per line (or something like this).

This might also be useful for multi-line literals of various types.

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Dec 7, 2017

Perhaps it's just my English.. So one could say that all the parameters in the examples mentioned here are aligned? Would "Make sure the lines containing the parameters of a method call have the same indentation" be more clear?

@deivid-rodriguez
Copy link
Contributor Author

ping @bbatsov. Do you think the message can be improved or is it just me not being a native speaker?

If I tell a group of people, "would you get aligned, please?", would they form a single vertical or horizontal row or they would make several rows or columns of variable number of people each 😝 ?

@mikegee
Copy link
Contributor

mikegee commented Mar 2, 2018

@deivid-rodriguez you are correct that a group of people getting aligned would imply single file. In a text document, alignment refers to the edge of the lines of text. Source code is typically left-aligned. See this screenshot from Microsoft Word's UI:
screen shot 2018-03-02 at 5 04 54 pm

@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Mar 2, 2018

The thing is the message is not refering to "line alignment" but to "parameter alignment", that's what I find confusing since as I see it whereas lines are aligned (they have the same number of leading whitespace), parameters are not (they are distanced differently to the beginning of the line).

That's why I think "Make sure all lines containing the parameters of a method call have the same indentation" or something along those lines would be more clear. At least I wouldn't have got wrong expectations about what this cop should do.

@deivid-rodriguez
Copy link
Contributor Author

Also, I never explained the motivation of this feature request.

The idea is to have cleaner diffs. With the "multiple parameter per line" style, diffs are not as clear and code edition is not as easy when you add or remove parameters, specially if you have line length restrictions. Well, and I also find it prettier, but that's not a very good motivation :)

@maxh
Copy link
Contributor

maxh commented Jun 20, 2018

This feature would be useful for my team well. I'd like to enforce option #3 in the Google JS Style Guide: https://google.github.io/styleguide/jsguide.html#formatting-function-arguments

I'm lightly exploring implementing a new cop myself. The docs at https://rubocop.readthedocs.io/en/latest/development/ seem pretty thorough, so that's probably where I'll begin. Of the existing cops, first_element_line_break.rb [0] looks somewhat related in that it deals with code being spit across multiple lines. Any other tips before I dive in? Thanks!

[0] https://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/cop/mixin/first_element_line_break.rb

@maxh
Copy link
Contributor

maxh commented Jun 20, 2018

Some interesting related discussion on the rufo repo: ruby-formatter/rufo#83, with some great research into patterns in the most popular Ruby repos.

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 19, 2018

There hasn't been much activity on this ticket and our Core Team is spread too thin on so many tasks, so I'll just close it for the sake of having a cleaner lists of tasks to focus on. We'd gladly review a PR, but it's unlikely that'd we're going to tackle this ourselves in the foreseeable future.

@bbatsov bbatsov closed this as completed Sep 19, 2018
@TSMMark
Copy link
Contributor

TSMMark commented Mar 9, 2019

Would love this cop, what would it take to implement? I wouldn't mind submitting a PR

@maxh
Copy link
Contributor

maxh commented Mar 10, 2019

I implemented this last year but haven't had a chance to try to upstream yet. Just created a PR:

#6824

We've been using it for ~6 months with much success, and think others might benefit. Please take look!

@TSMMark, I'm not sure the status of https://github.com/prettier/plugin-ruby, but it looks like it's matured a lot in the last 9 months, might offer what you're looking for.

maxh pushed a commit to maxh/rubocop that referenced this issue Mar 10, 2019
@maxh
Copy link
Contributor

maxh commented Mar 11, 2019

I also ended up needing to implement Layout/IndentMultilineClosingBrace to get the behavior I wanted here, just put that up for review: #6826.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants