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

Double-indent trailing closure bodies when using --closingparen same-line #667

Merged
merged 4 commits into from
Jul 16, 2020

Conversation

calda
Copy link
Collaborator

@calda calda commented Jul 12, 2020

This PR updates the indent rule to double-indent trailing closure bodies when using --closingparen same-line.

This is now valid indentation when using indent --indent 2 --closingparen same-line:

self.method(
  withParameter: 1,
  otherParameter: 2) { [weak self] in
    guard let error = error else { return }
    print("and a trailing closure")
}

If you use --closingparen balanced (the default), the trailing closure body stays single-indented:

self.method(
  withParameter: 1,
  otherParameter: 2
) { [weak self] in
  guard let error = error else { return }
  print("and a trailing closure")
}

@coveralls
Copy link

coveralls commented Jul 12, 2020

Coverage Status

Coverage increased (+0.01%) to 93.016% when pulling 85572ef on calda:cal--indent-closure-bodies into 3a131ff on nicklockwood:develop.

@nicklockwood
Copy link
Owner

I'm not sure if it makes sense to tie this to the --closingparen argument, but I don't have a better idea right now.

@nicklockwood nicklockwood merged commit 7d13875 into nicklockwood:develop Jul 16, 2020
@nicklockwood
Copy link
Owner

@calda would it be consistent with your style guide to indent the closing } as well for this scenario? i.e.

self.method(
  withParameter: 1,
  otherParameter: 2) { [weak self] in
    guard let error = error else { return }
    print("and a trailing closure")
  }

@calda
Copy link
Collaborator Author

calda commented Jul 17, 2020

I don't think we have an explicit preference for that. Both options seem fine to me personally (I'm not sure which I'd prefer if I had to pick one or the other)

@nicklockwood
Copy link
Owner

The latter is more consistent with how closures are handled in other scenarios, so I'd be more comfortable having that be the default behavior vs a double indent of just the body.

@calda
Copy link
Collaborator Author

calda commented Jul 17, 2020

I can make a follow-up PR that makes that the default, if you’d like.

@nicklockwood
Copy link
Owner

No, need, I just wanted to check it wouldn't render the feature unusable for you. Thanks 👍

Lukasz2891 pushed a commit to Lukasz2891/SwiftFormat that referenced this pull request Oct 23, 2020
leogdion pushed a commit to brightdigit/SwiftFormatSlim that referenced this pull request Nov 6, 2020
@nicklockwood
Copy link
Owner

@calda I ran into a case where this seemed like a bug (see #909) and I think I may need to tweak the rule, but I wanted to check with you first.

We already discussed indenting the closing }, which I forgot to do before but may do now. But also, would it be consistent with your style guide if it only double-indented for cases where the parameters are wrapped using before-first?

i.e. this would be double indented:

self.method(
  withParameter: 1,
  otherParameter: 2) { [weak self] in
    guard let error = error else { return }
    print("and a trailing closure")
  }

but this would not:

self.method(withParameter: 1,
            otherParameter: 2) { [weak self] in
  guard let error = error else { return }
  print("and a trailing closure")
}

I think this makes sense, since the purpose of the double indent (I assume) is to prevent the body lining up with the parameters, which isn't an issue in the second case. But let me know what you think.

@calda
Copy link
Collaborator Author

calda commented Apr 23, 2021

That makes sense to me! Our style guide only permits before-first, so we shouldn't be affected if you change the behavior for after-first.

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

Successfully merging this pull request may close these issues.

3 participants