Skip to content
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

Add a convenience init to ClosureCaptureSyntax #2127

Merged
merged 2 commits into from
Sep 2, 2023

Conversation

natikgadzhi
Copy link
Contributor

Summary

This adds a convenience init to ClosureCaptureSyntax that adds an equal token if the provided name is not nil.

It's supposed to be used by developers, so it omits the unexpected tokens.

Related: #1984

Copy link
Collaborator

@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.

Could you add an entry for this PR and #2112 to the new release notes file https://github.com/apple/swift-syntax/blob/main/Release%20Notes/510.md?

natikgadzhi added a commit to natikgadzhi/swift-syntax that referenced this pull request Aug 31, 2023
Provides release notes entries for:

- `ClosureCaptureSyntax.init` apple#2127
- `EnumCaseParameterSyntax.init`
  apple#2112
@natikgadzhi
Copy link
Contributor Author

@ahoppen done: 23c2098

@kimdv
Copy link
Collaborator

kimdv commented Sep 1, 2023

@swift-ci please test

Copy link
Contributor

@Matejkob Matejkob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates—great work! My internal nitpick radar 📡 kicked in, so I've left a few comments. Not sure how helpful they are, but there you go! 🙈

Comment on lines 11 to 18
- `ClosureCaptureSyntax.init(leadingTrivia: Trivia? = nil, specifier: ClosureCaptureSpecifierSyntax? = nil, name: TokenSyntax, equal: TokenSyntax = TokenSyntax.equalToken(), expression: some ExprSyntaxProtocol, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `ClosureCaptureSyntax` that takes a concrete `name` argument and automatically adds `equal = TokenSyntax.equalToken()` to it.
- Pull Request: https://github.com/apple/swift-syntax/pull/2127
- `EnumCaseParameterSyntax.init(leadingTrivia: Trivia? = nil, modifiers: DeclModifierListSyntax = [], firstName: TokenSyntax, secondName: TokenSyntax? = nil, colon: TokenSyntax = TokenSyntax.colonToken(), type: some TypeSyntaxProtocol, defaultValue: InitializerClauseSyntax? = nil, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `EnumCaseParameterSyntax` that takes a concrete `firstName` value and adds `colon = TokenSyntax.colonToken()` automatically to it.
- Pull Request: https://github.com/apple/swift-syntax/pull/2112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue related to these changes contains important information about the new APIs. I would consider adding a link to it, as stated in the template below. What do you think about it?

Suggested change
- `ClosureCaptureSyntax.init(leadingTrivia: Trivia? = nil, specifier: ClosureCaptureSpecifierSyntax? = nil, name: TokenSyntax, equal: TokenSyntax = TokenSyntax.equalToken(), expression: some ExprSyntaxProtocol, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `ClosureCaptureSyntax` that takes a concrete `name` argument and automatically adds `equal = TokenSyntax.equalToken()` to it.
- Pull Request: https://github.com/apple/swift-syntax/pull/2127
- `EnumCaseParameterSyntax.init(leadingTrivia: Trivia? = nil, modifiers: DeclModifierListSyntax = [], firstName: TokenSyntax, secondName: TokenSyntax? = nil, colon: TokenSyntax = TokenSyntax.colonToken(), type: some TypeSyntaxProtocol, defaultValue: InitializerClauseSyntax? = nil, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `EnumCaseParameterSyntax` that takes a concrete `firstName` value and adds `colon = TokenSyntax.colonToken()` automatically to it.
- Pull Request: https://github.com/apple/swift-syntax/pull/2112
- `ClosureCaptureSyntax.init(leadingTrivia: Trivia? = nil, specifier: ClosureCaptureSpecifierSyntax? = nil, name: TokenSyntax, equal: TokenSyntax = TokenSyntax.equalToken(), expression: some ExprSyntaxProtocol, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `ClosureCaptureSyntax` that takes a concrete `name` argument and automatically adds `equal = TokenSyntax.equalToken()` to it.
- Issue: https://github.com/apple/swift-syntax/issues/1984
- Pull Request: https://github.com/apple/swift-syntax/pull/2127
- `EnumCaseParameterSyntax.init(leadingTrivia: Trivia? = nil, modifiers: DeclModifierListSyntax = [], firstName: TokenSyntax, secondName: TokenSyntax? = nil, colon: TokenSyntax = TokenSyntax.colonToken(), type: some TypeSyntaxProtocol, defaultValue: InitializerClauseSyntax? = nil, trailingComma: TokenSyntax? = nil, trailingTrivia: Trivia? = nil)`
- Description: Provides a convenience initializer for `EnumCaseParameterSyntax` that takes a concrete `firstName` value and adds `colon = TokenSyntax.colonToken()` automatically to it.
- Issue: https://github.com/apple/swift-syntax/issues/1984
- Pull Request: https://github.com/apple/swift-syntax/pull/2112

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think linking the issue would be valuable.

Also, I think the full API signature isn’t that enlightening. What I would do instead is name the items

  • ClosureCaptureSyntax.init with a required name
  • EnumCaseParameterSyntax.init with a required firstName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about incorporating release notes into the DocC documentation for SwiftSyntax package? With this approach, it would be really easy to add direct links to specific methods right in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we would need to decide which module to put the release notes in. If we put it in the SwiftSyntax module, you can’t link to any API in e.g. SwiftParser. Since the release notes are for the entire package and not one specific module, I think having them at a top-level makes sense for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of this restriction that you can't link to any API from a different target. That's unfortunate. :/

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW apple/swift-docc#208 is tracking this restriction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both for the review! I'll clean up the changelog for now. Hopefully, I can get a generator-based version quickly enough and clean them up altogether in a few days.

Sources/SwiftSyntax/Convenience.swift Outdated Show resolved Hide resolved
Sources/SwiftSyntax/Convenience.swift Outdated Show resolved Hide resolved
**Summary**
This adds a convenience init to ClosureCaptureSyntax that adds equal
token if the provided `name` is not nil.

It's supposed to be used by developers, so it omits the unexpected
tokens.

Related: apple#1984
natikgadzhi added a commit to natikgadzhi/swift-syntax that referenced this pull request Sep 1, 2023
Provides release notes entries for:

- `ClosureCaptureSyntax.init` apple#2127
- `EnumCaseParameterSyntax.init`
  apple#2112

Co-authored-by: Mateusz Bąk <bakmatthew@icloud.com>
@natikgadzhi natikgadzhi force-pushed the syntax/convenience/closure-capture branch from 4d6c9b0 to 440b6da Compare September 1, 2023 20:40
Provides release notes entries for:

- `ClosureCaptureSyntax.init` apple#2127
- `EnumCaseParameterSyntax.init`
  apple#2112

Co-authored-by: Mateusz Bąk <bakmatthew@icloud.com>
@natikgadzhi natikgadzhi force-pushed the syntax/convenience/closure-capture branch from 440b6da to a9acb00 Compare September 1, 2023 20:42
@natikgadzhi
Copy link
Contributor Author

Alright! Should be good to go for this PR.

@ahoppen
Copy link
Collaborator

ahoppen commented Sep 1, 2023

@swift-ci Please test

@ahoppen
Copy link
Collaborator

ahoppen commented Sep 1, 2023

@swift-ci Please test Windows

@ahoppen
Copy link
Collaborator

ahoppen commented Sep 2, 2023

@swift-ci Please test macOS

@ahoppen
Copy link
Collaborator

ahoppen commented Sep 2, 2023

@swift-ci Please test Windows

@ahoppen ahoppen merged commit 6a00dbe into apple:main Sep 2, 2023
3 checks passed
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.

None yet

4 participants