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 pretty printing of multi-statement closures (issue #494) #498

Merged
merged 1 commit into from Apr 11, 2023

Conversation

stackotter
Copy link
Contributor

Issue #494 is related to pretty printing of multi-statement closures, and can be reproduced on main by formatting the following input:

{ a in a + 1
  a + 2
}

The pretty printer just spits out the snippet as is. This is because the first break is made elective instead of soft. Note that the closure is formatted correctly if a in is removed.

The fix that I propose (and have implemented) is forcing the break before the first statement to be soft if the closure contains multiple statements. I believe this is reasonable because there is no situation that I know of where Swift Format should format a multi-statement closure as a single-line closure ({ a + 1; a + 2 } is pretty un-idiomatic).

I have added a test for this change that fails on main and succeeds after my change.

This is my first contribution to Swift Format, so please let me know if there's anything I should change about this PR or future PRs (e.g. test style).

Copy link
Collaborator

@allevato allevato left a comment

Choose a reason for hiding this comment

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

Thank you!

Will kick off CI on the companion PR in swift-syntax after my other requests are done processing and merge when it's ready.

@stackotter
Copy link
Contributor Author

Awesome, thanks 👍

@allevato allevato merged commit c4d5e68 into apple:main Apr 11, 2023
allevato added a commit to allevato/swift-format that referenced this pull request Jun 29, 2023
Fix pretty printing of multi-statement closures (issue apple#494)
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.

None yet

2 participants