-
Notifications
You must be signed in to change notification settings - Fork 465
Unify PresentMaker and MissingNodesBasicFormatter, fixing a formatting bug in string literals
#1964
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
Unify PresentMaker and MissingNodesBasicFormatter, fixing a formatting bug in string literals
#1964
Conversation
|
@swift-ci Please test |
| ) | ||
| } | ||
|
|
||
| func testMultilineStringLiteralWithBlackLines() { |
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.
| func testMultilineStringLiteralWithBlackLines() { | |
| func testMultilineStringLiteralWithBlankLines() { |
I made a typo in the test method name.
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.
Haha, thanks for pointing it out.
baafe5c to
441b025
Compare
| assertFormatted( | ||
| source: #""" | ||
| assertionFailure(""" | ||
| First line |
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.
The blank line is gone. The code is the same as the test code above testMultilineStringLiteral().
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.
Oh, copying code from GitHub apparently drops empty lines 🤯 Thanks for catching it. I updated the test case
|
In the following test case (no spaces in the empty lines), an indent is inserted in the first blank line. This is not identical to the original string. assertFormatted(
source: #"""
assertionFailure("""
First line
Second line
""")
"""#,
expected: #"""
assertionFailure("""
First line
Second line
""")
"""#
)Actual Result: |
441b025 to
530547d
Compare
Fixed |
530547d to
3961b9c
Compare
|
@swift-ci Please test |
|
Running the following test case with the latest commit 3961b9c produces incorrectly indented results. Actual result: |
|
@swift-ci Please test |
…tting bug in string literals The underlying issues of the malformatted multi-line string literal are that 1. We are setting an `anchorPoint` for empty lines. Anchor points are meant to be starting points at which the user didn’t provide any manual indentation relative to which we should format the rest of the tree. But for empty lines it’s not like the user didn’t provide any indentation. There simply wasn’t anything to indent. 2. `leadingTrivia.trimmingTrailingWhitespaceBeforeNewline` was not considering whether the token text itself contained a newline in `isBeforeNewline`. Now, to detect if the token is followed by a newline, we should check if its text is empty, which is the case for empty string literals. Unfortunately, this is also the case for missing identifiers, which also have an empty text but whose empty text will be replace by a placeholder by `PresentMaker`. To distinguish between the two, I merged `PresentMaker` and `MissingNodesBasicFormatter` so that the `BasicFormat` also performs the text replacement to placeholders. Fixes swiftlang#1959 Co-Authored-By: kishikawa katsumi <[email protected]>
3961b9c to
653ae4b
Compare
|
Fixed as well. I can’t think of any more ways to break formatting of multi-line string literals but if you do find more, please let me know @kishikawakatsumi. And thanks for the thorough testing. |
|
@swift-ci Please test |
653ae4b to
0387483
Compare
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
The underlying issues of the malformatted multi-line string literal are that
anchorPointfor empty lines. Anchor points are meant to be starting points at which the user didn’t provide any manual indentation relative to which we should format the rest of the tree. But for empty lines it’s not like the user didn’t provide any indentation. There simply wasn’t anything to indent.leadingTrivia.trimmingTrailingWhitespaceBeforeNewlinewas not considering whether the token text itself contained a newline inisBeforeNewline.Now, to detect if the token is followed by a newline, we should check if its text is empty, which is the case for empty string literals. Unfortunately, this is also the case for missing identifiers, which also have an empty text but whose empty text will be replace by a placeholder by
PresentMaker. To distinguish between the two, I mergedPresentMakerandMissingNodesBasicFormatterso that theBasicFormatalso performs the text replacement to placeholders.Fixes #1959