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

rubocop multiline chaining #553

Closed
troessner opened this issue Jun 28, 2015 · 26 comments
Closed

rubocop multiline chaining #553

troessner opened this issue Jun 28, 2015 · 26 comments

Comments

@troessner
Copy link
Owner

See rubocop/rubocop#1591
I find the default rubocop strategy for this highly annoying and badly readable and would suggest that we go with something like:

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

WDYT?

@mvz
Copy link
Collaborator

mvz commented Jun 29, 2015

Are you referring to the indenting or to the dots?

@mvz
Copy link
Collaborator

mvz commented Jun 29, 2015

Uh, sorry, from the issue it's obviously the indenting.

@mvz
Copy link
Collaborator

mvz commented Jun 29, 2015

I tend to change that cop's setting to EnforcedStyle: indented so it alings like so, which matches what Vim does:

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

I would then probably move the first map to a separate line:

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

I'm not a fan of lining up at later points in the line since it involves a lot of hand-indenting.

@troessner
Copy link
Owner Author

Ok, then it's a tie.:)
pinging @chastell

@mvz
Copy link
Collaborator

mvz commented Jun 29, 2015

Note that I don't particulary like our current settings, which do:

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

@chastell
Copy link
Collaborator

I really dislike the dots at the end – my brain recognises them much more when they’re at the beginning and it’s so much more obvious that .map is a continuation from the previous line (map just looks like mis-indented).

@mvz
Copy link
Collaborator

mvz commented Jun 29, 2015

This issue is about the indenting @chastell, we can discuss the dots some other time 😄.

@chastell
Copy link
Collaborator

Ah, good point, I got side-tracked by the example. :)

Unfortunately, I dislike both

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

and

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

I’d either indent as deep as necessary to sync the identifiers (sorry!), or – which I would actually do in 99% of the cases – I’d introduce a temp variable so that I don’t have to wrap any lines. The number of lines would stay the same, but the brain won’t have to remember that the weirdly indented ones are continuations.

@mvz
Copy link
Collaborator

mvz commented Jun 30, 2015

Well, like I said, I would put the first map on its own line as well, which makes the identifier line-up moot.

Then, I prefer this:

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

Over this:

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

We can also disable this cop and allow this 😄 :

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

My vote would be to either use EnforcedStyle: indented or disable the cop entirely.

@chastell
Copy link
Collaborator

I’m ok with either as long as we agree on the general notion to introduce wraped lines only if temp variables can’t be used in an elegant way. :)

@troessner
Copy link
Owner Author

Ok, I'm lost. Did we agree on something now?
My preference would really be:

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

We can also go withh @mvz suggestion:

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

@chastell now it's up to you to say [1] or [2]. Or to suggest something completely different.:)

@chastell
Copy link
Collaborator

HEY I REALLY TRIED NOT TO PICK SIDES HERE

I find [1] more readable, but I understand [2]’s practicality (no need to hand-indent and it’s Vim default), although the lack of prefix dots throws me off a bit (I KNOW, I KNOW, IT’S A DIFFERENT DISCUSSION).

Or to suggest something completely different.

I would like to push the notion that in most cases we shouldn’t have this problem to begin with and simply introduce extra variables (rather than wrap the lines). :P

@troessner
Copy link
Owner Author

I find [1] more readable

So you're voting for [1], right?.:)

@mvz
Copy link
Collaborator

mvz commented Jul 1, 2015

@troessner I'm confused. Does [1] include the change in dots? Also, how would you format [1] if you moved the first map on its own line? Put another way, I like [1] a lot if I had to put the first map on the first line.

@troessner
Copy link
Owner Author

Does [1] include the change in dots?

The dots would be on the same line.

Also, how would you format [1] if you moved the first map on its own line?

No, I would have them on the same line. Haha.:)

Maybe we can compromise on this

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

?

@mvz
Copy link
Collaborator

mvz commented Jul 1, 2015

Maybe we can compromise on this

Hm, ok. That looks like a solution that RubuCop can enforce automatically. I'm not a fan of leading dots, but I can live with it.

@troessner
Copy link
Owner Author

Ok, then how about this:

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

cause that would work for me very well.:)

@PeterCamilleri
Copy link

A most interesting discussion so far with many good points. My vote goes for:

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

To me this is the most "symmetrical", but mostly the placement of the dots implies that the message is being sent to the value of the previous line.

@mvz
Copy link
Collaborator

mvz commented Jul 2, 2015

the placement of the dots implies that the message is being sent to the value of the previous line.

You could (and I do 😄) argue the other way around: The dot at the end of the line implies that the line is not done yet. The indenting at the start of the line implies that it is a continuation of the previous line.

@mvz
Copy link
Collaborator

mvz commented Jul 2, 2015

Ok, then how about this:

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

cause that would work for me very well.:)

Works for me 👍.

@PeterCamilleri
Copy link

Two points:
A) For me the beginnings of lines are much easier to visually scan than the ends of lines. Thus I prefer the continuation character (the dot) at the beginning.
B) It really does not matter what system is used so long as it is applied in a consistent manner. Things get ugly when different styles are used in the same code base.
C) I don't suppose that as a matter of style, this could have some configurability? At a drop dead minimum, the ability to turn it off.

@mvz
Copy link
Collaborator

mvz commented Jul 2, 2015

@PeterCamilleri we use RuboCop to enforce consistent dot placement. Reek itself does not check style issues. In our current RuboCop configuration, the trailing dot is enforced. RuboCop allows both styles.

Apart from that, I like to keep my lines short enough so I can scan both beginning and end. Otherwise, it is too easy to miss trailing dots, other continuation characters (operators etc.), and funny logic.

@PeterCamilleri
Copy link

My bad... Since this a discussion about reek, I was confused and thought this might be about something going into reek.

@troessner
Copy link
Owner Author

@mvz then let's configure rubocop accordingly. Whoever has time to look into this first .:)

@mvz
Copy link
Collaborator

mvz commented Jul 3, 2015

Done. Only makes a difference in about 3 places. WDYT, better or worse?

@troessner
Copy link
Owner Author

Better! Closing this issue..;)

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

4 participants