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

Issue when combining ClosingParenthesisIndentation and AlignParameters:with_fixed_indentation #1758

Closed
bquorning opened this issue Apr 4, 2015 · 15 comments

Comments

@bquorning
Copy link
Contributor

I like to configure Style/AlignParameters with EnforcedStyle: with_fixed_indentation, which would allow me code like this:

puts(1,
  foo: 'bar',
  one: 'two'
)

On the latest master (933d084), Style/ClosingParenthesisIndentation tells me to indent the closing parenthesis:

puts(1,
  foo: 'bar',
  one: 'two'
    )

I could move the 1 argument to a separate line, but I would rather keep the non-hash arguments on the same line as the method call, and the options hash on separate lines. Explicitly, I guess it would be:

puts(1, {
  foo: 'bar',
  one: 'two'
})
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 4, 2015

That's not a style worth supporting IMO as it's not exactly common. To be honest I've never seen Ruby code indented like this until just now...

@bquorning
Copy link
Contributor Author

I'm not sure how this code can be formatted then, given this combination of configurations. I could disable one of the cops, I guess :-)

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 5, 2015

Pretty sure the only two common indentation schemes for this bit of code are:

puts(1,
     foo: 'bar',
     one: 'two')

puts(
  1,
  foo: 'bar',
  one: 'two'
)

@digitalcora
Copy link

Just chiming in that I've seen this style used with some frequency when a method has a single initial parameter that is not a hash, followed by hash parameters. Example from some Factory Girl code in a Rails project:

create(:store,
  name: 'Downtown Boutique',
  address: '123 Main St, Anytown, USA'
)

To me at least, the equivalent cop-compliant code is harder to read, because the initial parameter is given the same visual "precedence" as the hash parameters, even though it is a completely separate and more important element:

create(
  :store,
  name: 'Downtown Boutique',
  address: '123 Main St, Anytown, USA'
)

I have no strong opinion on whether this exact style should be enforceable via Rubocop, but as it stands, merely using the style means that you have to forego ClosingParenthesisIndentation altogether. (I feel like maybe this should be treated as a bug, since taking the cop's suggestion literally results in a very odd style that I doubt was intended – @bquorning's second example in the first post)

@kaspernj
Copy link

+1

@alexdowad
Copy link
Contributor

Pretty sure the only two common indentation schemes for this bit of code are:

And one of the two you show is what @bquorning is asking for...

@alexdowad
Copy link
Contributor

And one of the two you show is what @bquorning is asking for

Whoops, I was wrong. Sorry, @bbatsov.

@alexdowad
Copy link
Contributor

I can code up a PR for this, but what should the config parameter which enables this style in Style/ClosingParenthesisIndentation be called?

@alexdowad
Copy link
Contributor

Ping.

Do we want this style added to ClosingParenthesisIndentation? Or is @bquorning just out of luck?

@jonas054
Copy link
Collaborator

Would this style also apply for method definitions and grouped expressions? I think we (or the person doing the implementation) need to see all examples before we can decide on what to call the configuration parameter/values. For reference, here's what's currently accepted:

      some_method(
        a
      )

      x = some_method(
        a
      )

      x =
        some_method(
          a
        )

      some_method(a
                 )

      some_method()

      def some_method(
        a
      )
      end

      private def some_method(
        a
      )
      end

      private def some_method(a
                             )
      end

      def some_method(a
                     )
      end

      def some_method()
      end

      w = x * (
        y + z
      )

      w = x * (y + z
              )

      w = x * (y + z +
              a)

      w = x * (y + z +
              a
              )

@aeberlin
Copy link

👍

@alexdowad
Copy link
Contributor

@bquorning, what do you think of @jonas054's comment? Are you looking for a new style specific to method calls, or should it be for grouped expressions (using parens) as well?

@bquorning
Copy link
Contributor Author

What I was looking for was well described by @Grantovich as

when a method has a single initial parameter that is not a hash, followed by hash parameters.

So yes, only for method calls.

@wjordan
Copy link

wjordan commented Mar 30, 2016

For some real-world examples of this style in the wild, see code-dot-org/code-dot-org#7562. (We're blacklisting the Style/ClosingParenthesisIndentation cop because of this issue.)

@gaffneyc
Copy link

gaffneyc commented Jun 7, 2016

We use this style quite a bit when we have one (or a few) non-hash params followed by a hash or keywords. Moving the non-hash arguments to the first line muddles the distinction between what is an argument and what is in the hash.

jonas054 added a commit to jonas054/rubocop that referenced this issue Jun 11, 2016
…meters

When the with_fixed_indentation style is used by Style/AlignParameters,
the hanging closing parenthesis should adapt. It's going to look
weird otherwise.
Neodelf pushed a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016
…meters (rubocop#3215)

When the with_fixed_indentation style is used by Style/AlignParameters,
the hanging closing parenthesis should adapt. It's going to look
weird otherwise.
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

9 participants