Factor out all leadingTrivia logic into Format
#645
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use format method in buildable nodes - Introduce a new variable for the built syntax for readability - Attach stored leadingTrivia separately in buildable nodes - Generate format methods for collection nodes - Move leading trivia fully into Format
|
@swift-ci please test |
|
@swift-ci please test macOS |
ahoppen
approved these changes
Aug 28, 2022
| @@ -7,9 +7,12 @@ final class ImportTests: XCTestCase { | |||
| let leadingTrivia = Trivia.unexpectedText("␣") | |||
| let identifier = TokenSyntax.identifier("SwiftSyntax") | |||
|
|
|||
| let importDecl = ImportDecl(path: AccessPath([AccessPathComponent(name: identifier)])) | |||
| let importDecl = ImportDecl( | |||
| leadingTrivia: leadingTrivia, | |||
There was a problem hiding this comment.
Actually, I don’t know why we always add the ␣ as leading trivia here. I think it’s something that was done in one of the first test cases and then just carried over to all the other tests. I would be totally fine to remove ␣ from all test cases because it’s rather weird anyway. I’m fine to keep it as-is in this PR though.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the entire
leadingTrivialogic intoFormat, thereby allowing for a cleaner separation of concerns: Thebuild...methods inBuildableNodes/BuildableCollectionNodesonly generate the syntax, theformat._formatmethods then deal with indentation.In particular, the
leadingTriviaparameter that was passed alongsideformat: Formatis no longer needed. Since many test cases, however, rely on this ad-hoc leading trivia generation, I have migrated as many tests as possible to theleadingTrivia-as-field. For smaller nodes, such as literals, which are heavily sugared through their respective convenience initializers, this would become too verbose, therefore I have removed the unexpected trivia node in those cases. Feel free to discuss this though!Some notes:
@_spi(Testing)for currently public indentation methods/properties onFormatwill come in a future PR since it depends on Add attributes to VariableDecl convenience initializers #644 and is mostly orthogonal to the changes in this PR.build...methods is that this becomes complicated for children that may or may not require leading newlines and due to some other complications such as not being able to switch over the parent'sSyntaxEnumbefore constructing its syntax (usingSyntaxKindwould have been an option, but would have required us to use the low-level SPI). In general, I think the overloaded_formatmethods are the more elegant solution here, also since we don't add a lot of unneeded empty trivias, but I would be curious to hear your thoughts on this!mainon debug builds ofgenerate-swift-syntax-builder.