Skip to content

Move TypeAttribute to SwiftSyntaxBuilder #942

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 10, 2022
Merged

Move TypeAttribute to SwiftSyntaxBuilder #942

merged 1 commit into from
Dec 10, 2022

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Oct 13, 2022 •
edited
Loading

@ahoppen

Let me know what you think.
My main idea is maybe to start removing the gyb dependency in this repo, so we only use Swift to generate the needed code.

@kimdv kimdv requested a review from ahoppen as a code owner October 13, 2022 18:48
kimdv
kimdv commented Oct 13, 2022
View reviewed changes
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from 51ae7d0 to e66f6b5 Compare October 13, 2022 18:55
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 5 times, most recently from 0164cbe to 920a2f6 Compare October 28, 2022 08:13
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 2 times, most recently from cdbf1ed to a7ffa43 Compare November 4, 2022 12:59
@kimdv
Copy link
Contributor Author

kimdv commented Nov 4, 2022

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 2 times, most recently from bcc9d23 to 85dd957 Compare November 22, 2022 19:42
ahoppen
ahoppen reviewed Nov 24, 2022
View reviewed changes
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 a lot for the continued de-gybification of SwiftSyntax. Overall looks good to me. I’ve got a few comments inline.

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 2 times, most recently from db08b01 to ba9ad40 Compare November 24, 2022 21:31
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 6 times, most recently from 2a66e65 to 72bc211 Compare December 5, 2022 12:10
@kimdv
Copy link
Contributor Author

kimdv commented Dec 5, 2022

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from 72bc211 to 883d1ca Compare December 5, 2022 12:29
@kimdv
Copy link
Contributor Author

kimdv commented Dec 5, 2022

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 2 times, most recently from 18d16d4 to e43fa39 Compare December 8, 2022 10:55
@kimdv
Copy link
Contributor Author

kimdv commented Dec 8, 2022

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from e43fa39 to 6c4c373 Compare December 8, 2022 12:08
kimdv
kimdv commented Dec 8, 2022
View reviewed changes
@kimdv
Copy link
Contributor Author

kimdv commented Dec 8, 2022

@swift-ci please test

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from 6c4c373 to 7e224da Compare December 9, 2022 09:22
@kimdv
Copy link
Contributor Author

kimdv commented Dec 9, 2022

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Dec 9, 2022

@ahoppen sadly I wasn't able to remove the AttributeKinds.py as first :(
It's used in the main swift repo.

So started just moving into SyntaxSupport

ahoppen
ahoppen reviewed Dec 9, 2022
View reviewed changes
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. Looks really good to me. As usual, I’ve got some comments though 😉

It’s a little unfortunate that we won’t be able to get all the way down to 0 gyb because AttributeKinds.py is shared with the Swift repo but maybe one day.

CodeGeneration/Sources/SyntaxSupport/gyb_helpers/utils.py
if hasattr(attr, 'code'):
parameters += ['code: %s' % attr.code]

if class_name not in ['ContextualSimpleDeclAttribute', 'ContextualDeclAttribute'] and hasattr(attr, 'swift_name'):
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to explicitly exclude ContextualSimpleDeclAttribute and ContextualDeclAttribute here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because those two don't use the swift_name in the parameters

https://github.com/apple/swift-syntax/blob/76d01195182593ff34f5ada1ab0910fae190fc9c/gyb_syntax_support/AttributeKinds.py#L29-L31

Copy link
Member

Choose a reason for hiding this comment

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

Oh, mind adding that as a short explanation comment for the next one who stumbles over it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 💪

@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from 7e224da to 8352e25 Compare December 9, 2022 11:52
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch 3 times, most recently from de40ad2 to 5c33f8f Compare December 9, 2022 13:28
@kimdv kimdv force-pushed the kimdv/replace-gyb-files-with-swift-syntax-builder branch from 5c33f8f to 7e80fa5 Compare December 9, 2022 14:41
@kimdv
Copy link
Contributor Author

kimdv commented Dec 9, 2022

@swift-ci please test

@kimdv
Copy link
Contributor Author

kimdv commented Dec 9, 2022

@swift-ci please test macOS

@kimdv kimdv merged commit b0fc9a0 into swiftlang:main Dec 10, 2022
@kimdv kimdv deleted the kimdv/replace-gyb-files-with-swift-syntax-builder branch December 10, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kimdv @ahoppen