-
Notifications
You must be signed in to change notification settings - Fork 441
Use a dedicated Token
type in SwiftSyntaxBuilder
#647
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 |
@swift-ci Please test macOS |
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.
Looks good, a few minor notes/questions below
Sources/generate-swift-syntax-builder/Templates/BuildableBaseProtocolsFile.swift
Show resolved
Hide resolved
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.
There is a space in the path name Convenience Initializers
Is that intended?
I stumbled on that too, I think in macOS-land it's not an uncommon convention to use spaces in folder names (see e.g. I think |
4bab48e
to
0d584d4
Compare
At the moment, tokens were a little bit of an oddity in SwiftSyntaxBuilder: All nodes were represented by dedicated types in SwiftSyntaxBuilder but tokens were represented by the `TokenSyntax` from the SwiftSyntax module. This has lead to a few problems: - We needed to add extensions on `TokenSyntax` to add convienience properties that are only designed for SwiftSyntaxBuilder - `TokenSyntax` was not `SyntaxBuildable` and thus couldn’t be used as elements in unexpected node collections In general, it seems cleaner if we just add a custom `Token` type in SwiftSyntaxBuilder. rdar://99030016
This allows up to pick up the `ExpressibleAs` conformances from ExpressibleAsConformances.py
0d584d4
to
37508b8
Compare
I didn’t put too much thought in it. But I agree that it’s easier for tooling in general if it doesn’t contain a space, so I changed it to |
@swift-ci Please test |
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.
Some minor stylistic comments below, otherwise this looks very good, thanks! I also like how we could unify Tokens.swift
, TokenSyntax.swift
and the new Token
type into a single file 👍
Sources/generate-swift-syntax-builder/Templates/TokenFile.swift
Outdated
Show resolved
Hide resolved
Sources/generate-swift-syntax-builder/Templates/TokenFile.swift
Outdated
Show resolved
Hide resolved
Sources/generate-swift-syntax-builder/Templates/TokenFile.swift
Outdated
Show resolved
Hide resolved
Sources/generate-swift-syntax-builder/Templates/TokenFile.swift
Outdated
Show resolved
Hide resolved
@swift-ci Please test |
At the moment, tokens were a little bit of an oddity in SwiftSyntaxBuilder: All nodes were represented by dedicated types in SwiftSyntaxBuilder but tokens were represented by the
TokenSyntax
from the SwiftSyntax module. This has lead to a few problems:TokenSyntax
to add convienience properties that are only designed for SwiftSyntaxBuilderTokenSyntax
was notSyntaxBuildable
and thus couldn’t be used as elements in unexpected node collectionsIn general, it seems cleaner if we just add a custom
Token
type in SwiftSyntaxBuilder.Also: Move the files that declare convenience initializers in SwiftSyntaxBuilder to their own directory.
rdar://99030016