Skip to content
This repository has been archived by the owner on Mar 1, 2020. It is now read-only.

change to leading dot configuration rather then trailing #6

Closed
wants to merge 1 commit into from

Conversation

BenR1312
Copy link
Contributor

We run into this error a lot:
Place the . on the previous line, together with the method call receiver.
Rather then this:

one.two.
  three

im suggesting we go back to what everyone likes doing in the first place which was

one.two
  .three

@wafendy
Copy link

wafendy commented Nov 22, 2017

Yes, please. Easier to move them around.

Copy link

@aniamoto aniamoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting cause it doesn't meet current community standards. Are you sure, team? Can we talk about it? ;p

@aniamoto
Copy link

aniamoto commented Nov 22, 2017

rubocop/ruby-style-guide#176
rubocop/ruby-style-guide#169 (comment)

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.

&&

Also, putting . on the second line precludes intervening comments in any version.

1.to_i
  # failure
  .to_s
1.to_i.
  # success
  to_s

@wafendy
Copy link

wafendy commented Nov 22, 2017

We are MoneySmart community, we should have our own opinion and not following other people standard blindly.

@aniamoto
Copy link

@wafendy these are community standards, it's good to use them because there will be new devs joining and they might be used to them as well. Also, I copied some valid arguments against this change in my previous comments.

@wafendy
Copy link

wafendy commented Nov 22, 2017

In expense of ignoring our argument why this works for us.

@aniamoto
Copy link

@wafendy because of this change we won't be able to copy code blocks to irb/pry (which I think might be used quite often, at least I use it). I wish we could discuss it on the tech talk because the cons are quite high, so I wish everyone who approves it was familiar with what changes it brings.

@aniamoto
Copy link

@BenR1312 are you ok to talk about it? :)

@wafendy
Copy link

wafendy commented Nov 22, 2017

That's a valid argument, then I'm ok to use the default setting.

@kinmor06
Copy link

i got no problem with either, so long as we do it consistently. personal choice, i go with trailing with the chained call indented

@BenR1312
Copy link
Contributor Author

https://github.com/bbatsov/ruby-style-guide#consistent-multi-line-chains

Adopt a consistent multi-line method chaining style. There are two popular styles in the Ruby community, both of which are considered good—leading . (Option A) and trailing . (Option B)

the resolution to rubocop/ruby-style-guide#169 (comment)

the resolution to the above is to actually include the idea that both styles are good and either is fine amongst the community
rubocop/ruby-style-guide@fefe83d

@anna id like you to talk about this one at the tech talk in particular this is a good issue thats been raised and id rather not have to talk about other peoples issues for them at a tech talk.
ill put it on the board since we have that now.

The reason why these PR's get put up in the first place is to act as a catalyst for discussion so if we think we need to talk about it further lets do that!

@BenR1312
Copy link
Contributor Author

We spoke about this at our Tech Talk and decided the benefits of the trailing dot far far outweigh the style preference of the leading dot.
Thanks to @aniamoto for bringing these points up.

@BenR1312 BenR1312 closed this Nov 28, 2017
@BenR1312 BenR1312 deleted the leading_dot_rather_then_trailing branch November 28, 2017 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants