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

Fix rules for indenting multiline assignments #723

Merged
merged 1 commit into from
Mar 14, 2015

Conversation

c-lliope
Copy link
Contributor

@jakecraige
Copy link
Contributor

👍

@gylaz
Copy link
Member

gylaz commented Mar 12, 2015

Do you mind also adding in to thoughtbot's config?

@gylaz
Copy link
Member

gylaz commented Mar 12, 2015

@salbertson What are your thoughts on also making this change in default? It would mean drifting away from RuboCop's default, but this is really an ugly setting. I fear I might be getting too personally attached to the defaults.

@salbertson
Copy link
Member

@gylaz, I don't think we should be affecting existing users by updating defaults, unless we think it's important enough and we communicate the change to everyone.

@salbertson
Copy link
Member

@Graysonwright, should this be added to the thoughtbot config too? Maybe in a different PR?

This looks good to me. 👍

@c-lliope c-lliope force-pushed the gw-fix-assignment-indentation branch from 603bad8 to d14104b Compare March 14, 2015 06:26
@c-lliope c-lliope merged commit d14104b into master Mar 14, 2015
@c-lliope c-lliope deleted the gw-fix-assignment-indentation branch March 14, 2015 06:26
c-lliope added a commit that referenced this pull request Mar 14, 2015
c-lliope added a commit that referenced this pull request Mar 18, 2015
@teoljungberg
Copy link
Contributor

Our styleguide is not updated for this https://github.com/thoughtbot/guides/blob/master/style/README.md#formatting
We should open a PR to thoughtbot/guides before changing it in Hound, everyone at thoughtbot doens't follow the developments made in Hound but they watch Guides.
I personally don't agree with this styleguide and would like it to be discussed, if the majority is in favor of this - we keep it. If not, let's chage it back to the defaults

ping @thoughtbot/hound @jferris @Graysonwright

@gylaz
Copy link
Member

gylaz commented May 5, 2015

@teoljungberg Just to clarify you prefer:

users = User.where(email: '[email protected]').
             update(user_parameters)

This PR changes this to favor:

users = User.where(email: '[email protected]').
  update(user_parameters)

And while it might not be explicitly in our guides, 99% of thoughtbot code follows the latter.

@gylaz
Copy link
Member

gylaz commented May 5, 2015

Actually, we do have a guideline for it:

Indent continued lines two spaces.

@teoljungberg
Copy link
Contributor

Actually no, this rule enforces this style (sorry for a shitty example, typing from my phone)

users = Users.
active.
awesome.
tall


Teo Ljungberg

On 5 maj 2015, at 22:18, Greg Lazarev [email protected] wrote:

@teoljungberg Just to clarify you prefer:

users = User.where(email: '[email protected]').
update(user_parameters)
This PR changes this to favor:

users = User.where(email: '[email protected]').
update(user_parameters)
And while it might not be explicitly in our guides, 99% of thoughtbot code follows the latter.


Reply to this email directly or view it on GitHub.

@gylaz
Copy link
Member

gylaz commented May 5, 2015

@teoljungberg Can you clarify where you're getting your information from? This comment rubocop/rubocop#1633 (comment) seems to be inline with what I've said, and the general spirit of this PR.

@teoljungberg
Copy link
Contributor

@gylaz I pinged you in the repo where we have the problem

@gylaz
Copy link
Member

gylaz commented May 6, 2015

I don't know what's going on on that related PR. I've added unit tests for this setting in #772.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants