Fix trivia handling issues in editor placeholder expansion #2511
Conversation
|
@swift-ci Please test |
|
@swift-ci Please test |
|
@swift-ci Please test Windows |
1 similar comment
|
@swift-ci Please test Windows |
| var leadingTriviaIndentation: Trivia | ||
| var trailingTriviaIndentation: Trivia |
There was a problem hiding this comment.
I believe since you removed default values in these properties we can change them to let's since they are not mutating anymore.
| var leadingTriviaIndentation: Trivia | |
| var trailingTriviaIndentation: Trivia | |
| let leadingTriviaIndentation: Trivia | |
| let trailingTriviaIndentation: Trivia |
| self.indentationStack = [self.indentationStack.first!] | ||
| self.anchorPoints = [:] | ||
| self.previousToken = nil | ||
| self.stringLiteralNestingLevel = 0 |
There was a problem hiding this comment.
NIT-PICK ALERT!
From what I can see we are not using self in other places in this class.
| self.indentationStack = [self.indentationStack.first!] | |
| self.anchorPoints = [:] | |
| self.previousToken = nil | |
| self.stringLiteralNestingLevel = 0 | |
| indentationStack = [indentationStack.first!] | |
| anchorPoints = [:] | |
| previousToken = nil | |
| stringLiteralNestingLevel = 0 |
There was a problem hiding this comment.
Oh, if only we were consistent about using self or not using self. But good catch, I removed it.
There was a problem hiding this comment.
Doesn't swift-format has a rule for that? 🤔
There was a problem hiding this comment.
No, because knowing whether self requires semantic knowledge. For example you can’t omit the self if there’s also a local variable with the same name.
ahoppen
force-pushed
the
ahoppen/trailing-trivia-whitespace
branch
from
ad014b0 to
d1765f8
Compare
|
@swift-ci Please test |
|
@swift-ci Please test |
|
@swift-ci Please test macOS |
| /// Clears all stateful data from this `BasicFormat`. | ||
| /// | ||
| /// This needs to be called between multiple `rewrite` calls to a syntax tree. |
There was a problem hiding this comment.
Could we just override rewrite and do this in there instead?
There was a problem hiding this comment.
rewrite currently isn’t open. And I don’t really want to make it open just for that reason… Thinking about this, I thought about introducing BasicFormat.format(_ node: some SyntaxProtocol) but that would be the same as SyntaxProtocol.formatted(using: BasicFormat). So, maybe we should just make reset internal and tell people to use SyntaxProtocol.formatted?
There was a problem hiding this comment.
Updated it to make reset internal again with the intention that you format code by calling SyntaxProtocol.formatted.
rdar://123287930
ahoppen
force-pushed
the
ahoppen/trailing-trivia-whitespace
branch
from
d1765f8 to
48f665c
Compare
|
@swift-ci Please test |
|
@swift-ci Please test |
rdar://123287930