Skip to content

Improve docs for implementing macros in an existing Swift package #184

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

Merged

Conversation

ianthetechie
Copy link
Contributor

This contribution improves the documentation for getting started with macros in an existing Swift Package. Notably, it is not obvious to first-time users that CompilerPluginSupport needs to be imported, and the default platform support is too low to support macros (in Xcode 15 at the time of this writing).

It also adds imports ahead of the FourCharacterCode macro implementation example, since these will be needed to make any(?) macros compile. Tangentially, it looks like #182 addresses the remaining things that tripped me up initially. These combined additions will smooth out the intro to macros significantly.

@amartini51 amartini51 self-requested a review September 26, 2023 18:21
Copy link
Member

@amartini51 amartini51 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 these additions! I took a pass at rewriting some of the prose.

Copy link
Contributor Author

@ianthetechie ianthetechie 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 review @amartini51! I've pushed a few changes.

  1. Accept your suggestion re: the bulleted list. Good call.
  2. I noticed that the use of ending punctuation was inconsistent. I added it, as that seemed to be the norm, but let me know if you'd prefer otherwise.
  3. I reorganized the part about adding the SwiftSyntax dependency to be in the list + Package.swift sample. That left the remaining original text a bit sparse (another single-sentence paragraph). I'd welcome any style suggestions on that.

@ianthetechie ianthetechie force-pushed the improve-macro-implementation-docs branch from 2506eef to 29dcb8e Compare November 15, 2023 13:19
@ianthetechie
Copy link
Contributor Author

Quick bump on this as it's getting a bit stale and now merge conflicts are starting to pop up.

@amartini51
Copy link
Member

@ianthetechie I'll take another look at this — I started a rewrite pass, but then had to focus on getting TSPL updated for Swift 5.9.2 beta 3. Thanks for resolving the merge conflict. I was expecting to see some conflicts, since other PRs also modified some of the same content as this one.

There's a balance in this PR — which I also had to make in the first version — between providing enough information about using Swift Syntax for folks to get started, and not trying to actually teach too much of Swift Syntax here. I'm also trying to make some adjustments to keep the order of steps linear, with alternate instructions at some points for new/existing code, rather than folding too many steps together under an "existing project" alternative.

A general workflow comment: force-pushing to a branch during PR review makes that review more difficult. As a reviewer, I can no longer easily see what changed since the last time I looked at the PR. I'm aware that workflows vary here, so you may have gotten the opposite guidance from others in the past. (For more details, please see CONTRIBUTING in this repo.)

@ianthetechie
Copy link
Contributor Author

Understood; sorry about that force push!

@amartini51
Copy link
Member

@ianthetechie How's my latest commit look to you? Some content that was recently merged into 'main' got deleted as part of merging that branch in here, which I restored. I also removed some duplicate discussion of SwiftSyntax. My intended approach was to keep the same step-by-step ordering in the content, but to add "if new" and "if existing" versions of the steps where needed.

Copy link
Contributor Author

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

Thanks @amartini51! I think this revision is much better, and I appreciate your attention to the flow of things. All looks good to me.

@amartini51 amartini51 self-requested a review November 27, 2023 15:55
@amartini51 amartini51 requested a review from bjlanier November 27, 2023 15:56
@amartini51 amartini51 merged commit c086922 into swiftlang:main Dec 4, 2023
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.

3 participants