Skip to content

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

Conversation

whiteio
Copy link
Contributor

@whiteio whiteio commented May 4, 2023

AccessPathSyntax should be renamed to ImportPathSyntax. This PR renames AccessPath to ImportPath, AccessPathComponent to ImportPathComponent, generates the code and adds the following type aliases to keep existing clients building:

  • AccessPathBuilder -> ImportPathBuilder
  • AccessPathSyntax -> ImportPathSyntax
  • AccessPathComponentSyntax -> ImportPathComponentSyntax

Resolves #1632

- Renamed AccessPath to ImportPath
- Renamed AccessPathComponent to ImportPathComponent

- Created the following type aliases to keep existing clients building
  - AccessPathBuilder -> ImportPathBuilder
  - AccessPathSyntax -> ImportPathSyntax
  - AccessPathComponentSyntax -> ImportPathComponentSyntax
@whiteio whiteio requested a review from ahoppen as a code owner May 4, 2023 20:01
Copy link
Member

@ahoppen ahoppen left a 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).

Comment on lines -19 to -20
case accessPathComponent(AccessPathComponentSyntax)
case accessPath(AccessPathSyntax)
Copy link
Member

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.

Comment on lines -19 to -20
case accessPathComponent
case accessPath
Copy link
Member

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.

@ahoppen
Copy link
Member

ahoppen commented May 4, 2023

@swift-ci Please test

@whiteio
Copy link
Contributor Author

whiteio commented May 6, 2023

@ahoppen The failing CI checks don't seem to be related to the changes in the PR, could you re-run the checks please?

@kimdv
Copy link
Contributor

kimdv commented May 6, 2023

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename AccessPathSyntax to ImportPathSyntax
3 participants