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

Insert white space before trailing closure of a macro expression #544

Merged
merged 2 commits into from
Jun 20, 2023
Merged

Insert white space before trailing closure of a macro expression #544

merged 2 commits into from
Jun 20, 2023

Conversation

kimberninger
Copy link
Contributor

This small PR fixes a bug causing the space before the trailing closure of a freestanding macro expression to be removed when running swift-format.

For example, the following line of code

let p = #Predicate<Int> { $0 == 0 }

would originally be formatted to

let p = #Predicate<Int>{ $0 == 0 }

In addition to resolving this issue, this PR adds some basic tests to ensure the correct behavior.

For more information and examples see also issue #542.

Copy link
Member

@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.

Thanks for the fix! Can you address the comment below and add test cases for it, similar to how they're done for function calls?

@@ -1250,6 +1250,10 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}

override func visit(_ node: MacroExpansionExprSyntax) -> SyntaxVisitorContinueKind {
before(
Copy link
Member

Choose a reason for hiding this comment

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

Before this, we need to pick up the same logic we're doing here: https://github.com/apple/swift-format/blob/8793d965ecd830bae26d86c6cddca607a3a7fa93/Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift#L981-L985

And then pass that value into forcesBreakBeforeRightDelimiter below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! I have pushed an update addressing the suggestion and adding another test case similar to the corresponding one in FunctionCallTests.swift.

assertPrettyPrintEqual(input: input, expected: expected, linelength: 40)
}

func testKeepWhiteSpaceAfterMacroNameWithGenerics() {
Copy link
Member

Choose a reason for hiding this comment

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

Given this test name, should the invocations below have a generic argument list? Or should the function name be changed?

IMO, you could just combine both cases into the same test:

#Preview {}
#Preview<Whatever> {}

and have a test that verifies that they keep the space, and then another test that verifies that if the space isn't there, it gets added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I seem to have mixed up the test methods' names 😅

But you are absolutely right, I agree that those should be merged to one test method each.

In the latest update I moved the tests for inserting the missing white space into #Preview{} and #Predicate<Int>{} to the same method and also expanded it by a third example #Preview("..."){} to show that the white space is also inserted when an argument list is supplied.
I did the same for the examples where the white space is already there and should be kept.

Furthermore, I renamed those methods to better convey their purpose.

let input =
"""
#Preview {}
#Preview("MyPreview) {
Copy link
Member

Choose a reason for hiding this comment

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

Here and throughout below, it looks like you're missing the trailing quote after "MyPreview, so we can't really trust the test results (if they do pass, the parser may be fixing the literal for us automatically)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the thorough attention, I would have totally missed that! Sorry for the inconvenience 🙇
The strings should now properly be closed with the latest update.

If a freestanding macro expression contains a trailing closure keep a,
single space before its opening brace or insert one if none exists.

Part of #542
Add a missing test case for pretty printing freestanding macro
expressions along with some basic tests checking that #542 has been
properly resolved in commit 7a43205.

The tests are by far not exhaustive and only cover the bug discovered in
issue #542.

Closes #542
Copy link
Member

@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.

We're good to go now, thank you!

@allevato allevato merged commit 4c88a60 into swiftlang:main Jun 20, 2023
allevato added a commit to allevato/swift-format that referenced this pull request Jun 29, 2023
…hite_space

Insert white space before trailing closure of a macro expression
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