-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
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
Since the |
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: 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. |
👎 for trailing dots. |
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. |
+1 for this pull request dot on beginning of line is inconstant with multi-line |
@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. |
@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. |
Yep, I like @jdalbert suggestion as well. |
👍 on @jdalbert's suggestion. |
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 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. |
jdalbert, +1 |
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.) |
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 ( Either(operation).
>= { successful_method }.
>= { failful_operation } |
i concur with the argument against. |
Leading dots are an antipattern:
|
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. |
@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.
|
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). |
@prusswan You can always copy the fragment without the trailing dot to irb easily, but not with leading ones. |
@whitequark that is not a regular part of my workflow so no comments on that, but |
@prusswan I don't think leading/trailing dots have any effect whatsoever on debugging and breakpoints |
I can easily switch/comment out fragments and still retain a valid syntax. Autotest mechanism takes care of the execution. |
|
@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. |
|
I'm 👎 on this one. |
👎 it makes no sense to me. |
I believe this style is misguiding. |
@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. |
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:
The pros and cons are kind of long, so I’ll just link to them on Stack Overflow. |
@bbatsov I see the change: rubocop/rubocop@e41175f Is the default to be beginning of line or end of line? Seems to be. Thanks. |
Yes. See |
@focused commented on Jun 9, 2013, 2:21 AM PDT:
I've always seen it in Haskell as (looks better with 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 |
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 |
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
|
@kashif-umair I think you may as well just indent line 2 and 3, making |
@bbatsov can you please put option C in the style guide? It is a nice compromise.
|
In any language where line terminators are mandatory, like Java, the benefits of the leading However, in any language where line terminators are optional, like Ruby, the benefits of the trailing |
Very interesting discussion! Personally I always had a very strong preference for the first option with the leading Never thought of the option Many thanks to everyone for the lengthy and thorough discussion. |
Ah wouldn't it be just so great if Ruby could read syntax like this one
it would be a nice compromise (that would looks better than the one mixing As far as I know the 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
|
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.
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