-
Notifications
You must be signed in to change notification settings - Fork 470
Insert a space before colon in ternary expression in Fix-It #1641
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
Conversation
|
@swift-ci please test |
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibachan, welcome to SwiftSyntax’s development.
I would prefer to adjust the formatting in BasicFormat instead of ParseDiagnsoticsGenerator because that way we also get the correct formatting you you are constructing a ternary operator e.g. from SwiftSyntaxBuilder, which is what people will do when writing macros.
In that case, it should work both when the operators are folded and when they haven’t been folded yet (see https://swiftpackageindex.com/apple/swift-syntax/main/documentation/swiftoperators).
An unfolded ternary operator would be the following
SequenceExprSyntax {
BooleanLiteralExprSyntax(true)
UnresolvedTernaryExprSyntax(firstChoice: IntegerLiteralExprSyntax(1))
IntegerLiteralExprSyntax(0)
}and a folded ternary operator would be
TernaryExprSyntax(
conditionExpression: BooleanLiteralExprSyntax(true),
firstChoice: IntegerLiteralExprSyntax(1),
secondChoice: IntegerLiteralExprSyntax(0)
)The code that you’d like to change is most likely in here https://github.com/apple/swift-syntax/blob/cecb0340a95f00f8f2725c912b494266ed7b4a80/Sources/SwiftBasicFormat/BasicFormat.swift#L177
|
Thank you for your feedback. I'll take a look into it! |
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks great! I’ve got one slight comment on how to improve implementation, otherwise this is ready to be merged. 👍🏽
Co-authored-by: Alex Hoppen <alex@alexhoppen.de>
ahoppen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, @gibachan. This looks good to me. Once CI passes, I’ll merge the PR.
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
This pull request addresses #1607