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

Standardize on the way method chains are broken up. #289

Closed
gylaz opened this issue Jan 15, 2015 · 4 comments
Closed

Standardize on the way method chains are broken up. #289

gylaz opened this issue Jan 15, 2015 · 4 comments

Comments

@gylaz
Copy link
Contributor

gylaz commented Jan 15, 2015

There are currently several ways in Ruby to break up long method chains.

First:

stub_request(:put, url).
  with(
    headers: {
      "Authorization" => "some_token",
      "Accept" => "some_media_type",
    }
  )

Second:

stub_request(:put, url).with(
  headers: {
    "Authorization" => "some_token",
    "Accept" => "some_media_type",
  }
)

I believe we prefer the first -- it makes it easier to see that the arguments are for with. We also have a guideline that backs this:

If you break up a chain of method invocations, keep each method invocation on its own line. Place the . at the end of each line, except the last

However, what if we need to chain something else to these calls:

stub_request(:put, url).
  with(
    headers: {
      "Authorization" => "some_token",
      "Accept" => "some_media_type",
    }
  ).
  to_return(
    body: "some body text",
    status: 200
  )

That looks awkward, but it follows the style of the "First" scenario. Naturally we'd do this:

...
      "Accept" => "some_media_type",
    }
  ).to_return(
    body: "some body text",
    status: 200
  )

but then that break consistency with how with is broken to its own line.

We could impose a rule like, always bring the closing paren and the dot, when chaining. Resulting in:

stub_request(
  :put,
  url
).with(
  headers: {
  "Authorization" => "some_token",
  "Accept" => "some_media_type",
  }
).to_return(
  body: "some body text",
  status: 200
)

My question is: is it worth standardizing one way for things like this? Or is "use your best judgement" a good enough solution here?

@jferris
Copy link
Member

jferris commented Jan 19, 2015

I haven't been feeling much pain from this. I think I've been writing this version:

stub_request(:put, url).
  with(
    headers: {
      "Authorization" => "some_token",
      "Accept" => "some_media_type",
    }
  ).
  to_return(
    body: "some body text",
    status: 200
  )

It's consistent with all the guidelines, it's consistently formatted, and it's easy to scan for method names in the chain.

@jferris
Copy link
Member

jferris commented Mar 5, 2015

I'm going to close this, as the discussion seems to have wrapped up.

@gylaz want to open a pull request to solidify this?

@jferris jferris closed this as completed Mar 5, 2015
@mcmire
Copy link

mcmire commented Mar 6, 2015

FWIW, I agree with this, we should make this a guideline. I've seen the second form lately and was irked.

@gylaz
Copy link
Contributor Author

gylaz commented Mar 7, 2015

Actually, we already have a guideline that covers this:

If you break up a chain of method invocations, keep each method invocation on its own line. Place the . at the end of each line, except the last.

Source

However, I'm not sure how it's suppose to work in a case like this:

  some_really_really_long_method_name(
    arg1: "foo",
    arg2: "bar",
    arg3: "baz",
  ).
  another_method_name_with_args(
    arg1: "one",
    arg2: "two",
    arg3: "three",
  )

Should the another_method_name_with_args method be indented one level (but that would look weird)?
We always indent subsequent method in chains like:

  User.where(name: "blah").
    order("name DESC").
    includes(:subscriptions, :address).
    limit(10)

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

No branches or pull requests

3 participants