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

include '.' on first line of multiline chained method invocations #176

Closed
wants to merge 1 commit into from

Conversation

lcowell
Copy link

@lcowell lcowell commented May 28, 2013

This changes where the . lives from the second line to the first line in the expression. Having no . at the end of the first line makes that line appear to be complete, but you must read ahead to the next line in order to see that the expression continues.

This closes issue #169

This changes where the '.' lives from the second line to the first line in the expression. Having no '.' at the end of the first line makes that line appear to be complete, but you must read ahead to the next line in order to see that the expression continues.

This closes issue rubocop#169
@bbatsov
Copy link
Collaborator

bbatsov commented May 29, 2013

Since the . is at the beginning of the next line you're hardly required to do any reading ahead. Leading dots just stand out. I've never seen evidence that the alternative style you suggest is more popular than the one currently suggested.

@lcowell
Copy link
Author

lcowell commented May 30, 2013

You're right, you hardly have to read ahead, but you DO have to read ahead - and so does ruby. My argument against the next line dot is:
-it doesn't work at all in 1.8 (some people still use it)
-there is nothing to tell the reader to look at the next line (more ambiguous IMHO)
-it has a small performance penalty (https://gist.github.com/lcowell/3a63edecad51177857ec)

What do you think we should do ? I'd say our reasons are about equal. I like the trailing dot because it tells me to read the next line, you like the dot on the leading dot to make it stand out more.

Is there some process to handle situations like this or does the repo owner win ? I'd suggest that we include the pros and cons to each method in the guide, but then it has little value to include it at all and should probably just be eliminated altogether. Most importantly, let's not forget that our goal is to make ruby better and help people write better ruby code.

@rafamoreira
Copy link

👎 for trailing dots.

@jeromedalbert
Copy link
Contributor

I don't know about you guys but I like dots on the second line when combined with indentation :

# Example 1
one.two.three
       .four

# Example 2
my_array.select { |str| str.size > 5 }
        .map    { |str| str.downcase }

Identation + dot on the second line aligned with the previous one help me know visually (thus immediately) what is going on. There is no reading ahead in these cases.

@equivalent
Copy link
Contributor

+1 for this pull request

dot on beginning of line is inconstant with multi-line or or and #169 (comment)

@bbatsov
Copy link
Collaborator

bbatsov commented May 30, 2013

@lcowell It's impossible to cater to everyone's preferences. If we simply list the two options available we're not actually recommending anything - and that's what style guides are for. Anyways, I'll keep this open for a while to hear what other people think as well. It's clear the opinions are greatly divided on that particular point.

@lcowell
Copy link
Author

lcowell commented May 31, 2013

@bbatsov agreed. No sense in including something in the guide if we're not making a clear recommendation. I was trying to make that same point above.

I like what @jdalbert was suggesting. Lining the dot up with the line above is an improvement does a better job of drawing the eye down than unaligned indentation. This addresses my primary concern with what's currently in the guide.

@bbatsov
Copy link
Collaborator

bbatsov commented May 31, 2013

Yep, I like @jdalbert suggestion as well.

@lee-dohm
Copy link
Contributor

👍 on @jdalbert's suggestion.

@ghost
Copy link

ghost commented May 31, 2013

I've been using trailing dots, with the second line indented.

As stated by others, the point is that when you read that line, you know that the statement continues on the next line. To me, that's more natural than reading a statement that looks complete in itself but actually continues in an oh-by-he-way fashion.

@jdalbert's solution fixes the problem with aesthetics but doesn't change the fact that the best place to indicate continuation of that statement is before you get to the next line.

# bad - need to consult first line to understand second line
one.two.three.
  four

# good - it's immediately clear what's going on the second line
one.two.three
  .four

I have two problems with this rationale. For one, you have to read both lines no matter what. You can't read four or .four and have any clue what's going on without reading the preceding line. The only way in which the second line might help with that is that it starts with a ., but if it's really just a matter of visually indicating that the previous line needs to be looked at, I think the fact that the line is indented does this fairly effectively.

Lastly, the second example is helpful only so far as you're reading or scanning the code from bottom to top -- which I suppose you could -- but I don't get why readability in that direction would be preferred over readability from top to bottom.

We want to make it clear that the second line goes with the first, but we don't want to make it clear that the first line goes with the second. This seems backwards. Trailing dot and second line indentation addresses the issue from either direction. This is a no-brainer if you ask me.

@fuadsaud fuadsaud mentioned this pull request May 31, 2013
@focused
Copy link

focused commented Jun 6, 2013

jdalbert, +1

@xjlu
Copy link

xjlu commented Jun 9, 2013

In real world programming, we have to read all lines anyway. The order of these method calls may or may not matter. With leading dots, editing is a bit easier, particularly when you need to add a new method call or change the order. (Similar to the javascript issue, you always have to remember to remove/add the trailing comma, so some developers prefer to put 'comma' in the beginning of each property.)

@focused
Copy link

focused commented Jun 9, 2013

When I start using an awesome gem for monads monadic, I realized that sometimes it is better to write dot at the end of line. Look (>= is an alias for bind):

Either(operation).
  >= { successful_method }.
  >= { failful_operation }

@philoserf
Copy link

i concur with the argument against.

#176 (comment)

bbatsov referenced this pull request in rubocop/rubocop Jul 3, 2013
@whitequark
Copy link

Leading dots are an antipattern:

  • They require the continuation to be present at precisely next line. (I.e. you cannot put more than one in-line comment in between.)
  • They break mental context by requiring one to always analyze one more line of code.
  • Most importantly, code written with leading dots cannot be copied and pasted to irb/pry.

@fuadsaud
Copy link
Contributor

fuadsaud commented Jul 3, 2013

@whitequark

  1. This is important.
  2. I don't understand why you wouldn't want to analyse the whole line. It's a single statement that would possibly be in one single line if not for it's length.
  3. Really neat, didn't think about that.

Anyway, I agree with @dingle, this is pretty similar to the leading/trailing comma in hashes. Each style has it's pros and cons; it's a matter of taste.

@whitequark
Copy link

@fuadsaud (2) Oh no, this is not what I meant. If the style guide forbids these leading dots, then I in my entire codebase, if a statement does not have a trailing dot, I know it ends right there.

Disagree on two counts.

  • Unlike comma in hashes, leading dots have objective differences--at least two of the three I recounted.
  • The very purpose of a style guide is to enforce a particular subjective style; and most often it makes sense to be conservative. You've already banned some constructs on similar grounds, e.g. and/or, or some %-literals, and rightly so--the resulting code is easier to read for everyone.

@prusswan
Copy link

prusswan commented Jul 5, 2013

I find leading dots more natural, because the first line (along with any number of lines that follow) will all be valid terminable statements. I feel this makes debugging and editing easier. This is also consistent with the idea of method-chaining (valid statements formed on top of valid statements).

@whitequark
Copy link

@prusswan You can always copy the fragment without the trailing dot to irb easily, but not with leading ones.

@prusswan
Copy link

prusswan commented Jul 5, 2013

@whitequark that is not a regular part of my workflow so no comments on that, but debugger and breakpoints work well enough for me.

@whitequark
Copy link

@prusswan I don't think leading/trailing dots have any effect whatsoever on debugging and breakpoints

@prusswan
Copy link

prusswan commented Jul 5, 2013

User.where
# .joins(...)
# ...
.group(...)
# ...

I can easily switch/comment out fragments and still retain a valid syntax. Autotest mechanism takes care of the execution.

@xjlu
Copy link

xjlu commented Jul 5, 2013

@whitequark

  1. I am not sure if it's still the case regarding the limitation of inline comments. Isn't that considered as a bug if it is as you described?
  2. The issue with irb/pry: isn't it a limitation of irb/pry? It doesn't make sense that we let a tool gem define a language's semantics.

@fuadsaud
Copy link
Contributor

fuadsaud commented Jul 5, 2013

@dingle the problem with irb is: when you paste the first line, irb receives the EOL and interprets the content as a full statement; that's the expected behaviour. When it sees the trailing dot, it interprets the content as a incomplete statement, therefore waiting for the rest to be pasted until it evaluates the code.

@whitequark
Copy link

@dingle

$ ruby -e $'1 # bar\n# baz\n  .display'
-e:3: syntax error, unexpected '.', expecting end-of-input
  .display
   ^
$ ruby -e $'1 # bar\n  .display'
1
$ ruby -v
ruby 2.0.0p0 (2013-02-24 revision 39474) [x86_64-linux]

@agis
Copy link

agis commented Aug 6, 2013

I'm 👎 on this one.

@rafaellima
Copy link

👎 it makes no sense to me.

@ddsferreira
Copy link

I believe this style is misguiding.
I also believe we should avoid long method chains.
And provided we have self contained method chains I would prefer a single line approach:
foo(arg1).bar(arg2).baz(arg3, arg4)
I would also add a maximum configuration on this upon which an error/alert should be raised by rubocop.
In my opinion the default maximum should be 3.

@elliottmason
Copy link

@kballenegger 's approach is arguably a nice compromise between the two styles. The trailing slash indicates that the statement continues, while still allowing one to use preceding dots, and it still works in IRB. Additionally, it doesn't require any configuration changes to Rubocop. I'm happy with it.

I agree that we should be avoiding long method chains in most scenarios. However, sometimes it's not the number of methods being chained that causes one to have to continue a statement on the next line, but merely the number of characters in a statement.

@roryokane
Copy link

I have compiled all the arguments I could find in this thread into a comprehensive list of pros and cons. All four styles described in this thread are compared:

  • . at line end
  • . at line beginning
  • . at line beginning, indented to the previous .
  • \ at line end, . at next line’s beginning

The pros and cons are kind of long, so I’ll just link to them on Stack Overflow.

@justin808
Copy link

@bbatsov I see the change: rubocop/rubocop@e41175f

Is the default to be beginning of line or end of line? Seems to be.

Thanks.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 9, 2015

s the default to be beginning of line or end of line? Seems to be.

Yes. See default.yml for alternatives.

@bb010g
Copy link

bb010g commented May 13, 2015

@focused commented on Jun 9, 2013, 2:21 AM PDT:

When I start using an awesome gem for monads monadic, I realized that sometimes it is better to write dot at the end of line. Look (>= is an alias for bind):

Either(operation).
  >= { successful_method }.
  >= { failful_operation }</pre></div>

I've always seen it in Haskell as (looks better with bind instead of >= here IMHO)

monadic_operation.bind ->(foo)
  {bar(foo, 1)}.bind ->(biz)
  {baz(biz, 2)}

which would be equivalent to the following in Haskell do-notation

do
  foo <- monadicOperation
  biz <- bar foo 1
  baz biz 2

@kashif-umair
Copy link

I have a question about blocks with these chaining methods. I'm using Rails so my example is specific to Rails but rails is Ruby after all. So here it is:

user.posts.includes(:comments).where(published: true).where.not(post_category: 'Private').each do |post|
  # do something with post here
end

What is the best way to write the above code? I do something like this:

user.posts.includes(:comments).
where(published: true).
where.not(post_category: 'Private').each do |post|
  # do something with post here
end

I prefer to use trailing dots but if look at my code, I have not indented line number 2 and 3 because if I indent them then I have to add one more indentation level to each block.

@equivalent
Copy link
Contributor

equivalent commented Aug 24, 2016

I would say:

user
  .posts
  .includes(:comments)
  .where(published: true)
  .where.not(post_category: 'Private')
  .each do |post|
     # do something with post here
  end

or

user.
  posts.
  includes(:comments).
  where(published: true).
  where.not(post_category: 'Private').
  each do |post|
     # do something with post here
  end

I would prefer the first version

reason for having it in separate lines is that it's easier to alter (e.g. you add .order(:created_at) the commit message will be just one line in git log instead of long line where is not clear what have changed.

.where.not are together on a same line as they are logically bound together

@FranklinYu
Copy link

I have not indented line number 2 and 3 because if I indent them then I have to add one more indentation level to each block.

@kashif-umair I think you may as well just indent line 2 and 3, making each block indented one more level, because that makes sense: one level for continuation, another for nesting. In ruby code, I believe indenting 5 levels are very common.

@cyoce
Copy link

cyoce commented Jan 24, 2017

@bbatsov can you please put option C in the style guide? It is a nice compromise.

one \
.two

@desmond-silveira
Copy link

In any language where line terminators are mandatory, like Java, the benefits of the leading . outweigh the benefits of the trailing .. It's just easier for a human to see the . at the beginning of a line than at the end.

However, in any language where line terminators are optional, like Ruby, the benefits of the trailing . outweigh the benefits of the leading .. The ability to paste code into a REPL is just too important.

@mhenrixon
Copy link

Very interesting discussion! Personally I always had a very strong preference for the first option with the leading . but for me it is just how my brain is wired. It is also how I prefer to work with other languages like SQL where I prefer the , to be in the beginning of the line to more easily be able to add and remove lines without having to also worry about adding a trailing comma somewhere.

Never thought of the option C that is definitely my new preference!

Many thanks to everyone for the lengthy and thorough discussion.

@Startouf
Copy link

Startouf commented Apr 6, 2017

Ah wouldn't it be just so great if Ruby could read syntax like this one

some_data.
.map { |d| d**2 }.
.sort_by { |d| d.odd? }.
.reject { |d| d.zero? }.
.reduce(:+)

it would be a nice compromise (that would looks better than the one mixing \and .)

As far as I know the .. operator is only used for date ranges but never broken by a \n so shouldn't that be theoretically feasible in future versions of Ruby ?

Also, and this most likely a personal feeling, but I don't like the indentation after the first line breaks, whatever the choice for the placement of the dot. But in case of leading dots it feels obvious enough that the chain continues from the previous lines that I don't see the pros of indentation.

On the contrary I would argue extra indentation makes it harder to close blocks. Take the following example of chaining with multiline blocks (putting aside the code smell), it makes it harder to realize if you're missing an end or not. I've quite several times struggled to find the missing end of code blocks, and having code that offsets itself for no reason makes it more error-prone. Maybe my linters are just bad, but often I would just get a red rubocop error at the end of the file and I'd have to find the missing/extra end myself

def get_questions
  Question.
    where(:score.gte => 42).
    pluck(:answer).
    select do |answer|
      answer.is_the_answer_to_life_and_everything?
    end.uniq
  # If not paying attention I'd want to put an `end` here
end

marcotc added a commit to wealthsimple/ws-style that referenced this pull request Feb 12, 2019
There's a long discussion (rubocop/ruby-style-guide#176 (comment)) in the Rubocop community around this and the consensus is to "be consistent", no matter if you choose trailing or leading dots.

At Wealthsimple there's a considerable disadvantage to using leading dots: code with leading dots cannot be pasted into irb/pry. This affects local testing and scripting.

We'd had bugs in the past related to people pasting large snippets from a perfectly functioning Ruby file into pry, but that script fails very subtly, amidst hundreds of successfully pasted lines, one fails but the whole block is still successful.
Example:
```ruby
# a lot of code here
list = bar.map{|x|x+1}
         .select{|x|x>1}
# much more code here
```
this code still "works", but won't give you the correct result.
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

Successfully merging this pull request may close these issues.