Skip to content

Add fix and tests for postfix pound ifs #383

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 2 commits into from
Aug 22, 2022
Merged

Add fix and tests for postfix pound ifs #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85c9757
Add fix and tests for postfix pound ifs
Jul 20, 2022
08e1943
Use private function instead of extension on Syntax
Aug 22, 2022
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
30 changes: 28 additions & 2 deletions Sources/SwiftFormatPrettyPrint/TokenStreamCreator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,7 +1308,15 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
before(tokenToOpenWith.nextToken, tokens: .break(breakKindClose, newlines: .soft), .close)
}

if let condition = node.condition {
if isNestedInPostfixIfConfig(node: Syntax(node)) {
before(
node.firstToken,
tokens: [
.printerControl(kind: .enableBreaking),
.break(.reset),
]
)
} else if let condition = node.condition {
before(condition.firstToken, tokens: .printerControl(kind: .disableBreaking))
after(
condition.lastToken,
Expand Down Expand Up @@ -3460,7 +3468,11 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {

if let calledMemberAccessExpr = calledExpression.as(MemberAccessExprSyntax.self) {
if calledMemberAccessExpr.base != nil {
before(calledMemberAccessExpr.dot, tokens: [.break(.contextual, size: 0)])
if isNestedInPostfixIfConfig(node: Syntax(calledMemberAccessExpr)) {
before(calledMemberAccessExpr.dot, tokens: [.break(.same, size: 0)])
} else {
before(calledMemberAccessExpr.dot, tokens: [.break(.contextual, size: 0)])
}
}
before(calledMemberAccessExpr.dot, tokens: beforeTokens)
after(expr.lastToken, tokens: afterTokens)
Expand All @@ -3485,6 +3497,20 @@ fileprivate final class TokenStreamCreator: SyntaxVisitor {
}
}

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

while this?.parent != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check all the ancestors, not just the immediate parent? Is this required to make nested clauses have the right indentation? (If it is, that's fine, I just wanted to verify the reason.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I was seeing, yes we need to check all the ancestors. In the code to handle the break before the #if the ancestor was consistent but it was a great-grandparent, not the immediate parent.

But in the code to ensure that the indentation is correct the ancestor that was PostfixIfConfigExprSyntax changed based upon how many modifiers were chained together.

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

this = this?.parent
}

return false
}

extension Syntax {
/// Creates a pretty-printable token stream for the provided Syntax node.
func makeTokenStream(configuration: Configuration, operatorContext: OperatorContext) -> [Token] {
Expand Down
160 changes: 160 additions & 0 deletions Tests/SwiftFormatPrettyPrintTests/IfConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,164 @@ final class IfConfigTests: PrettyPrintTestCase {

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

func testPostfixPoundIfAfterParentheses() {
let input =
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
}
"""

let expected =
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for a situation where you would have multiple members accesses inside the #if block? For example:

 #if os(iOS)
    .foo()
    .bar()
  #endif

We want to make sure the .bar() line (and any subsequent lines) don't get additional indentation; they should be the same as .foo().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: https://github.com/apple/swift-format/pull/383/files#diff-e366fdb32497e39bd5e8e8f666541fb18fbded18ad436459b26665555fd1e190R261

#endif
.commonModifier()
}

"""

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

func testPostfixPoundIfAfterParenthesesMultipleMembers() {
let input =
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
.anotherModifier()
.anotherAnotherModifier()
#endif
.commonModifier()
.anotherCommonModifier()
}
"""

let expected =
"""
VStack {
Text("something")
#if os(iOS)
.iOSSpecificModifier()
.anotherModifier()
.anotherAnotherModifier()
#endif
.commonModifier()
.anotherCommonModifier()
}

"""

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

func testPostfixPoundIfNested() {
let input =
"""
VStack {
Text("something")
#if os(iOS) || os(watchOS)
#if os(iOS)
.iOSModifier()
#else
.watchOSModifier()
#endif
.iOSAndWatchOSModifier()
#endif
}
"""

let expected =
"""
VStack {
Text("something")
#if os(iOS) || os(watchOS)
#if os(iOS)
.iOSModifier()
#else
.watchOSModifier()
#endif
.iOSAndWatchOSModifier()
#endif
}

"""

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


func testPostfixPoundIfAfterVariables() {
let input =
"""
VStack {
textView
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
}
"""

let expected =
"""
VStack {
textView
#if os(iOS)
.iOSSpecificModifier()
#endif
.commonModifier()
}

"""

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

func testPostfixPoundIfAfterClosingBrace() {
let input =
"""
HStack {
Toggle(isOn: binding) {
Text("Some text")
}
#if !os(tvOS)
.toggleStyle(SwitchToggleStyle(tint: Color.blue))
#endif
.accessibilityValue(
binding.wrappedValue == true ? "On" : "Off"
)
}
"""

let expected =
"""
HStack {
Toggle(isOn: binding) {
Text("Some text")
}
#if !os(tvOS)
.toggleStyle(
SwitchToggleStyle(tint: Color.blue))
#endif
.accessibilityValue(
binding.wrappedValue == true
? "On" : "Off"
)
}

"""

assertPrettyPrintEqual(input: input, expected: expected, linelength: 45)
}
Copy link
Member

Choose a reason for hiding this comment

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

A test that uses nested #ifs would be great too, since they have a somewhat interesting underlying node structure:

Text("...")
  #if os(iOS) || os(watchOS)
    #if os(iOS)
      .iOSModifier()
    #else
      .watchOSModifier()
    #endif
    .iOSAndWatchOSModifier()
  #endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: https://github.com/apple/swift-format/pull/383/files#diff-e366fdb32497e39bd5e8e8f666541fb18fbded18ad436459b26665555fd1e190R294

}