Skip to content

Fix more postfix pound if scenarios #402

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Apr 11, 2023
Merged

Fix more postfix pound if scenarios #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4cad9a8
Fix more postfix pound if scenarios
Sep 17, 2022
fdc708f
Simplify the implementation
Sep 17, 2022
1361dd5
Simplify again
Sep 18, 2022
d7b390f
Fix formatting of postfix pound ifs
Feb 6, 2023
4fd796f
Improve implementation
Feb 6, 2023
3172157
Merge branch 'apple:main' into fixMorePostfixPoundIfs
DavidBrunow Feb 7, 2023
dea0d25
Fix build issues after bringing in the latest from main
Feb 7, 2023
79b9d3a
Fix more build errors
Feb 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 43 additions & 8 deletions Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,11 +1331,24 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}

if isNestedInPostfixIfConfig(node: Syntax(node)) {
let breakToken: Token
let currentIfConfigDecl = node.parent?.parent?.as(IfConfigDeclSyntax.self)

if let currentIfConfigDecl = currentIfConfigDecl,
let tokenBeforeCurrentIfConfigDecl = currentIfConfigDecl.previousToken,
isNestedInIfConfig(node: Syntax(tokenBeforeCurrentIfConfigDecl)) ||
tokenBeforeCurrentIfConfigDecl.text == "}" {
breakToken = .break(.reset)
} else {
breakToken = .break
before(currentIfConfigDecl?.poundEndif, tokens: [.break])
}

before(
node.firstToken,
tokens: [
.printerControl(kind: .enableBreaking),
.break(.reset),
breakToken,
]
)
} else if let condition = node.condition {
Expand Down Expand Up @@ -3461,17 +3474,39 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}

private func isNestedInPostfixIfConfig(node: Syntax) -> Bool {
var this: Syntax? = node
var this: Syntax? = node

while this?.parent != nil {
if this?.parent?.is(PostfixIfConfigExprSyntax.self) == true {
return true
}
while this?.parent != nil {
// This guard handles the situation where a type with its own modifiers
// is nested inside of an if config. That type should not count as being
// in a postfix if config because its entire body is inside the if config.
if this?.is(TupleExprElementSyntax.self) == true {
return false
}

this = this?.parent
if this?.is(IfConfigDeclSyntax.self) == true &&
this?.parent?.is(PostfixIfConfigExprSyntax.self) == true {
return true
}

return false
this = this?.parent
}

return false
}

private func isNestedInIfConfig(node: Syntax) -> Bool {
var this: Syntax? = node

while this?.parent != nil {
if this?.is(IfConfigClauseSyntax.self) == true {
return true
}

this = this?.parent
}

return false
}

extension Syntax {
Expand Down
108 changes: 86 additions & 22 deletions Tests/SwiftFormatPrettyPrintTests/IfConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,10 @@ final class IfConfigTests: PrettyPrintTestCase {
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
}

"""
Expand All @@ -277,13 +277,13 @@ final class IfConfigTests: PrettyPrintTestCase {
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
.anotherModifier()
.anotherAnotherModifier()
#endif
.commonModifier()
.anotherCommonModifier()
#if os(iOS)
.iOSSpecificModifier()
.anotherModifier()
.anotherAnotherModifier()
#endif
.commonModifier()
.anotherCommonModifier()
}

"""
Expand Down Expand Up @@ -311,14 +311,14 @@ final class IfConfigTests: PrettyPrintTestCase {
"""
VStack {
Text("something")
#if os(iOS) || os(watchOS)
#if os(iOS)
.iOSModifier()
#else
.watchOSModifier()
#if os(iOS) || os(watchOS)
#if os(iOS)
.iOSModifier()
#else
.watchOSModifier()
#endif
.iOSAndWatchOSModifier()
#endif
.iOSAndWatchOSModifier()
#endif
}

"""
Expand All @@ -343,10 +343,10 @@ final class IfConfigTests: PrettyPrintTestCase {
"""
VStack {
textView
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
}

"""
Expand Down Expand Up @@ -390,4 +390,68 @@ final class IfConfigTests: PrettyPrintTestCase {

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

func testPostfixPoundIfBetweenOtherModifiers() {
let input =
"""
EmptyView()
.padding([.vertical])
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
"""

let expected =
"""
EmptyView()
.padding([.vertical])
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not handled properly by these changes because I didn't think to try adding this until I was creating this pull request. Will look into how to solve this but wanted to have it in this pull request so I didn't forget to mention it.

Copy link
Member

Choose a reason for hiding this comment

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

Have you had any luck with this one? For consistency we'd definitely like if the #if aligns with the preceding . and then the contents are indented one more level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not yet, things at work got really busy soon after I created this. I'll take another look at it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took me a bit longer than I expected to get back to this. I think this latest commit works as intended. Let me know if I misunderstood.

I don't love the implementation I landed on so I'l be looking to improve it if I can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happier where this is now. Would appreciate any and all feedback :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@allevato Friendly ping for awareness, not urgency, here since it took me so long to get back to this. I think it is behaving how you described now.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay; I lost track of this one. These results look great, thanks for getting all these cases working so well!


"""

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

func testPostfixPoundIfWithTypeInModifier() {
let input =
"""
EmptyView()
.padding([.vertical])
#if os(iOS)
.iOSSpecificModifier(
SpecificType()
.onChanged { _ in
// do things
}
.onEnded { _ in
// do things
}
)
#endif
"""

let expected =
"""
EmptyView()
.padding([.vertical])
#if os(iOS)
.iOSSpecificModifier(
SpecificType()
.onChanged { _ in
// do things
}
.onEnded { _ in
// do things
}
)
#endif

"""

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