-
Notifications
You must be signed in to change notification settings - Fork 440
Rename AccessPathSyntax
to ImportPathSyntax
#1638
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
Rename AccessPathSyntax
to ImportPathSyntax
#1638
Conversation
- Renamed AccessPath to ImportPath - Renamed AccessPathComponent to ImportPathComponent - Created the following type aliases to keep existing clients building - AccessPathBuilder -> ImportPathBuilder - AccessPathSyntax -> ImportPathSyntax - AccessPathComponentSyntax -> ImportPathComponentSyntax
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 to me. ✅
I left a few review comments for myself because this will somewhat become the blueprint of which compatibility functions we need to create when renaming a syntax node kind and I want to keep track of where we intentionally continue to break source compatibility (which is fine, we just want to minimize the impact as much as possible).
case accessPathComponent(AccessPathComponentSyntax) | ||
case accessPath(AccessPathSyntax) |
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.
Note to myself (no action required on your side) since this is the first time we are trying to do a rename while maintaining source compatibility as much as possible and I’m trying to assess where source breakages might occur.
We can’t rename a type without also renaming the case in SyntaxEnum
. I think that it might be fine because there are way fewer people switching over a SyntaxEnum
than there are using the type names.
case accessPathComponent | ||
case accessPath |
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.
Same here, just a note to myself, no action required on your side: This is not source compatible if we are renaming a syntax node’s type name. We might be able to mitigate this by defining a static member on SyntaxKind
as follows
public extension SyntaxKind {
@available(*, deprecated, renamed: "importPath")
var accessPath: SyntaxKind { .importPath }
}
This way we will only break clients that are exhaustively switching over SyntaxKind
, which is fairly unlikely since the enum is so huge.
@swift-ci Please test |
@ahoppen The failing CI checks don't seem to be related to the changes in the PR, could you re-run the checks please? |
@swift-ci please test |
AccessPathSyntax
should be renamed toImportPathSyntax
. This PR renamesAccessPath
toImportPath
,AccessPathComponent
toImportPathComponent
, generates the code and adds the following type aliases to keep existing clients building:AccessPathBuilder
->ImportPathBuilder
AccessPathSyntax
->ImportPathSyntax
AccessPathComponentSyntax
->ImportPathComponentSyntax
Resolves #1632