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

Align the operands of an expression in an assignment spanning multiple lines bug (or me not understanding the config) #1633

Closed
edwardloveall opened this issue Feb 4, 2015 · 26 comments

Comments

@edwardloveall
Copy link

I'm running: 0.28.0 (using Parser 2.2.0.2, running on ruby 2.0.0 x86_64-darwin14.0.0)

With the code:

users = User.where(email: 'user@example.com').
             update_all(user_parameters)

I get the rubocop warning:

Align the operands of an expression in an assignment spanning multiple lines

It passes with this which I feel isn't correct indentation:

users = User.where(email: 'user@example.com').
        update(user_parameters)

I'm not sure if this is a bug or my error.

@jonas054
Copy link
Collaborator

jonas054 commented Feb 5, 2015

Style/MultilineOperationIndentation can be configured with the aligned style (default), which enforces

users = User.where(email: 'user@example.com').
        update(user_parameters)

and indented, which enforces

users = User.where(email: 'user@example.com').
  update(user_parameters)

The aligned style is supported by ruby-mode 1.1 in Emacs and indented is another common style.

So I wouldn't say that there's a bug in RuboCop here. The behavior is intended. To support your style, which I wouldn't rule out at this point, we'd have to add a third alternative. Maybe call is semantic since it tries to convey a meaning relating to a call chain? I have a suspicion there might be a few corner cases with this new style that I can't describe in detail right now, but I don't know.

@edwardloveall
Copy link
Author

I see. Thanks for the explanation. I could have sworn this wasn't the case in previous versions? Anyway, I understand it's a weird problem. I think I was looking at it like "aligning method calls", as you said, as opposed to multiline operations in general.

Just sort of thinking out loud. Feel free to close if you don't think it's a good idea to add.

@raphaelpra
Copy link

Being able to verticaly align method calls when they are related can ease the understanding, in my opinion.

For instance with this dummy exemple:

a = [1, 3, 5].map { |i| i + 1 }
             .map { |i| i + 2 }

It seems to me more self explanatory than the rubocop compliant:

a = [1, 3, 5].map { |i| i + 1 }
    .map { |i| i + 2 }

In rails, this situation happen when you have severals query/scope:

@users = User.active
             .visible
             .where(is_admin: true)    

@matthewford
Copy link

I agree with raphaelpra on this one, it's quite common to align them that way in rails

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 24, 2015

@rafalchmiel Why not simply use one method call per line?

a = [1, 3, 5]
    .map { |i| i + 1 }
    .map { |i| i + 2 }

You should keep in mind this is expression continuation and the expression starts with the first receiver. I understand your reasoning, but it seems flawed to me. There's also the fact few editors support the indentation you suggest.

@matthewford
Copy link

Would that translate to this?

@users = User
         .active
         .visible
         .where(is_admin: true)    

I'm not sure that's easier to read than the version above.

@ghost
Copy link

ghost commented Feb 24, 2015

@bbatsov, I think you mean @raphaelpra 😄.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 24, 2015

@rafalchmiel Sorry about this.

@maxshelley
Copy link

There's also the fact few editors support the indentation you suggest.

This is actually what brought me to this issue. For me, Sublime doesn't correctly use syntax highlighting on the object when I align the code like @matthewford's example.

@raphaelpra's example seems to be common in Rails, I see it a lot, but it's failing in Rubocop.

@ghost
Copy link

ghost commented Mar 13, 2015

👍

@dblessing
Copy link

I'm completely OCD about code style, but I can't reconcile this rule with my OCD. :( Sorry, but I disagree with this cop. I'll disable it completely for now but I hope you'll re-evaluate based on all this feedback.

@nathanpalmer
Copy link

Looks like were in the same boat. We're going to disable the rule for now, which I would rather not do, but it seems that neither indented or aligned will work with chaining like this

results = scope.where(facebook_accounts_connections: { account_id: params[:account_id] })
               .order("facebook_connections.created_at desc")
               .page(page)
               .per(page_size)

It reads a lot easier when all of the methods that are chained are lined up so you can easily see what is connected to what. I'll discuss with the team on the alternative mentioned here as well. It's definitely not what we're used too.

@ryanbillingsley
Copy link

Count me as one looking for support for the style @nathanpalmer and @raphaelpra pointed out. 👍

@kevgo
Copy link

kevgo commented Sep 18, 2015

Another vote to support the style proposed by @nathanpalmer and @raphaelpra. We use this style a lot, in a variety of languages. It is concise, vertical-space efficient, and makes abundantly clear even with a quick glance how the call chain is structured. As an example a definition of strong parameters in Rails:

params.require(:objective)
      .permit(:due_date, :title, :key_results)

@jeromedalbert
Copy link

👍 for @nathanpalmer, @raphaelpra and @kevgo 's style

@fabiosammy
Copy link

👍 for @nathanpalmer and @raphaelpra style

@johncalvinyoung
Copy link

👍 the dot-aligned style @nathanpalmer and @raphaelpra pointed out. We use it heavily in Rails projects, also this similar syntax when dealing with long lists of symbol parameters.

delegate :name,
         :count,
         :budget,
         to: :project_plan

@saneshark
Copy link

👍

1 similar comment
@akshayrawat
Copy link

👍

@digitalbias
Copy link

👍 for @nathanpalmer and @raphaelpra style. We use it alot in our code as well.

@globalkeith
Copy link

👍 Thanks for adding this style:

model
  .exchange_draw_shares
  .includes(includes_query)
  .joins(:share)
  .where.not(share_id: previous_ids)

IMHO This enables the source of the pipeline to be easily recognised.

@saneshark
Copy link

Yes, noticed that this is working and it's great! Allows for comments as well.

model
  .exchange_draw_shares                          # complicated scope logic
  .includes(includes_query)                      # this eager loads associations that we need
  .joins(:share)                                 # the shares join table
  .where.not(share_id: previous_ids)             # where no previous shares exist

big 👍

@jonas054
Copy link
Collaborator

@globalkeith @saneshark You're welcome, but this style has been accepted since the introduction of Style/MultilineOperationIndentation in v0.27.0. 😄 What we're discussion now is the addition of

model.exchange_draw_shares
     .includes(includes_query)
     .joins(:share)
     .where.not(share_id: previous_ids)

as an accepted alignment. It's added in the pull request you see mentioned above, but hasn't been merged yet.

@maia
Copy link

maia commented Dec 22, 2015

Both indented and aligned use a different style than Rubymine (version 8.x, but I think it hasn't changed compared to previous versions) does when copy-pasting. Rubymine will use:

array = Model
            .includes(something)
            .joins(:blah)
            .where.not(some_id: ids)

Is this a style that should be considered as bug of Rubymine, or should it be supported by Rubocop as additional style? I find it nicer than both the rubocop aligned style:

array = Model
        .includes(includes_query)
        .joins(:share)
        .where.not(share_id: previous_ids)

and the rubocop indented style:

array = Model
  .includes(includes_query)
  .joins(:share)
  .where.not(share_id: previous_ids)

@jonas054
Copy link
Collaborator

Does the Rubymine style align the dot with the last character of the first line? We need more examples if we are to support it. I suggest adding a separate issue, but we have to wait and see what happens with #2493 first.

@maia
Copy link

maia commented Dec 25, 2015

Rubymine does not align with the last character of the first line, but instead uses an indentation of 4 spaces:

test = Abc
           .hello
           .world

test = Abcd
           .hello
           .world

test = Abcde
           .hello
           .world

test = Abcdef
           .hello
           .world

test = Abcdefg
           .hello
           .world

and:

Abc
    .hello
    .world

Abcd
    .hello
    .world

Abcde
    .hello
    .world

Abcdef
    .hello
    .world

Abcdefg
    .hello
    .world

(let me add that in all other cases Rubymine uses the standard 2 spaces as indentation, there must be a reason why they double that in multilines)

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