-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Specify method chaining dot style rule #146
Conversation
@@ -29,6 +29,7 @@ Formatting | |||
brace on its own line. | |||
* Indent continued lines two spaces. | |||
* Indent private methods equal to public methods. | |||
* Place the `.` on the same line as the method name when splitting up long lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think adding an example here would help clarify the meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I provided a link to an example. I do think it's useful and more relevant inline, but I'm cautions as it doesn't seem to be the convention to link to examples for each rule.
Do we try to line up the dots? foo.bar
.baz |
@JoelQ From the rule above, I would think not. Lining up the dots has the same problem as lining up tokens. |
I've always liked indenting one level in cases like this def example
foo.bar
.baz
.whatevs
end |
I like this, but maybe the guideline can be reworded just a bit to be more understandable? When I first read it I was a bit confused -- you've already got the dot on the same line... oh you mean lines that come after the first line, I see. Perhaps something like this:
|
@hgmnz Yes, that's how it should be -- as per style/samples/ruby.rb. I had a type, in the PR description, indenting only one space. |
Ah ok :) Instead of "put it at the same level as the method" I'd say
Don't tie it to the method definition: def method
if do_it
foo.bar
.baz
end
end |
I actually prefer to put the dot on the same line as the method, in part because Vim knows to auto-indent. Example: one.
two.
three I agree on indenting rules, I think: +2 spaces for every item in the chain except the originator. |
I prefer the trailing dot. I'm pretty sure the leading dot failed to parse in older versions of Ruby (not that we care, really). |
I also prefer the trailing dot (e.g what Gabe said). For me, it is the clue that says "hey, this continues on to the next line" while reading. |
+1 for the trailing dot |
This looks clearer def method_name
foo
.bar
.baz
end IMHO the leading dot looks less confusing. |
I think this is one of those situations where you'll get used to whatever you see most often, and it's important that we always do it the same way so you know where to look for it. There's a bunch of discussion on the pros and cons of each style here: rubocop/ruby-style-guide#176 I created a poll to help decide this issue within thoughtbot: https://docs.google.com/a/thoughtbot.com/forms/d/1dxvamONQnQ0pJhIKDg4A0z8-ha0Aavi3r3wguRJR8pA/viewform |
I'm one of the commenters over there. My current thoughts can be summarized by goto fail
goto fail That bug was only possible because the code relied on the convenience of omitting brackets for one-line if statements. Making a leading dot your style is like never bracketing one-line if statements. At some point, someone will introduce a stupid bug and would be impossible if you just used trailing dot/brackets. Also, @whitequark rocks |
@bf4, I assume this is what you're referring to: rubocop/ruby-style-guide#176 (comment) |
(@jferris yes) |
Leading dots can be harder to read in a diff. Here's an example: https://github.com/thoughtbot/whetstone/compare/v2...jf-dependencies#diff-3 |
This discussion has been going on for a long time, and there are passionate people on both sides. To me, that means that we should close this and explicitly not have guidelines around this. |
Gabe, the poll has an option "I don't think we should have a guideline." I don't want to avoid having a guideline every time we can't come to a near-unanimous decision. I think we'll generally sacrifice consistency that way. I also think this particular example requires consistency. I think either style will be easy enough to read, but not knowing which style to expect reduces readability greatly. |
If there is an unanimous decision, why even bother having a guideline? :) |
I agree with @jferris on this one: this is such an arbitrary thing, we just have to pick one. @whitequark because we hire people. |
@mike-burns Re: Leading dots can be harder to read in a diff -- wouldn't indenting the lines that continue the chain fix that? |
@gabebw Also re: vim-ruby doesn't know how to indent leading dots -- it should as of two months ago: vim-ruby/vim-ruby@a7b0b38 (in fact I just tested it out). FWIW, I would vote for leading dots just because we follow the same style over in JavaScript/CoffeeScript-land. |
@mcmire We have a voting form for this issue. Did you get a chance to place your vote on that? |
Here's the form to capture opinion votes (link). |
I voted on the poll. Doesn't matter to me trailing or leading but I do want to see one or the other picked. I'll get used to either convention. |
@gylaz Yeah I did... just wanted to clarify why exactly. |
👍 for revised wording. |
If we break any, I think we should break them all. Example: # Yes
foo.
bar.
baz
# No
foo.bar.
baz If you see a chain of methods on multiple lines, it would be easy to miss the extra call on the first line. |
@@ -37,6 +40,7 @@ Formatting | |||
* Use [Unix-style line endings] (`\n`). | |||
* Use [uppercase for SQL key words and lowercase for SQL identifiers]. | |||
|
|||
[dot guideline example]: https://github.com/thoughtbot/guides/blob/master/style/samples/ruby.rb#L11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the ruby sample need to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To include more lines, you mean? Or to place the dot in the right place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, I didn't realize the line already showed this convention
What about this revised, revised wording given the feedback?
|
Okay, looks good to me now 👍 |
The example in the commit message no longer matches the example in the commit. Good to merge other than that. |
When splitting up a chain of method calls, use a trailing `.` and place each method on its own line ```ruby foo. bar. baz ```
Just walked around and saw this issue. That's cool that you decided to use trailing dot. I haven't seen that someone mentioned the main pro. 😃 The biggest thing about this way, is that such code can be simply copy-pasted to repl, while with leading dot this can not be done without concating lines in editor. |
When splitting long lines place the dot on the same line as the method: