Skip to content

DocC setup #36

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
merged 6 commits into from
Aug 5, 2022
Merged

DocC setup #36

merged 6 commits into from
Aug 5, 2022

Conversation

yim-lee
Copy link
Member

@yim-lee yim-lee commented Jul 28, 2022

No description provided.

@yim-lee yim-lee requested a review from tomerd July 28, 2022 22:16
@ktoso
Copy link
Member

ktoso commented Aug 1, 2022

We figured out a slightly better way to deal with the package files -- could you make files for "old swift versions" and just keep one for the new ones as Package.swift? This way we don't have to keep adding new files for every new Swift release in the future. See our doc about the migration for details.


- ``StatsdClient/makeCounter(label:dimensions:)``
- ``StatsdClient/makeRecorder(label:dimensions:aggregate:)``
- ``StatsdClient/makeTimer(label:dimensions:)``
Copy link
Member

Choose a reason for hiding this comment

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

do we need to adda few more here?

Copy link
Member Author

Choose a reason for hiding this comment

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

added init and shutdown. do you have others in mind?

Copy link
Member

Choose a reason for hiding this comment

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

makeGauge also?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't see makeGauge?

Copy link
Member

Choose a reason for hiding this comment

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

I guess this API is all coming from swift-metrics, its a protocol implementation

Copy link
Member

@ktoso ktoso Aug 3, 2022

Choose a reason for hiding this comment

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

There isn't a makeGauge but Gauge calls into makeRecorder (for better or worse that's how it is)

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM looks good to me now!

@tomerd
Copy link
Member

tomerd commented Aug 2, 2022

I think the "trick" with the Package.swift is not actually the right way. @fabianfett and myself went through this with the lambda-runtime and iirc it break downs in some scenarios. @fabianfett do you recall the details?

@Lukasa
Copy link

Lukasa commented Aug 2, 2022

I don't think we're playing a trick here.

Concretely, the observed behaviour of SwiftPM around the @swift-x.y package manifests is utterly baffling. SwiftPM seems to have a heuristic that combines the version in the filename with the tools version in order to select what manifest to use. However, this proposal clearly does work: if the tools version in Package.swift is newer than the tools version in any other manifest, SwiftPM will prefer it for future releases.

@tomerd
Copy link
Member

tomerd commented Aug 2, 2022

here is the history of going back and forth on this with aws-lambda-runtime: swift-server/swift-aws-lambda-runtime#254 (comment)

iirc things worked okay locally and in CI, but broke when consuming the package as a dependency which is why we rolled it back. @fabianfett may recall more details

@Lukasa
Copy link

Lukasa commented Aug 2, 2022

The breakage appears to be the result of using symlinks, which we aren't doing elsewhere.

@Lukasa
Copy link

Lukasa commented Aug 2, 2022

Concretely, the documentation explains a behaviour, but that behaviour demonstrably diverges from what SwiftPM actually does. Notice that the documentation has both filenames and tools versions. The tools versions appear to bear more of the burden than the filenames.

@ktoso
Copy link
Member

ktoso commented Aug 3, 2022

Hmmm, I can confirm that the "comment version" takes precedence over what the filename says, I've seen that.

I can also confirm having a Package.swift with tools version 5.6 is picked up correctly by a 5.7 swift, so that all seems to work like we'd want to -- and what the current pattern carters to (so old versions with @ and new one without).

@tomerd
Copy link
Member

tomerd commented Aug 4, 2022

@Lukasa we agree the way this is documented and works is confusing. @abertelrud @neonichu could you chime in here on the original design, and what is the recommended approach?

in any case, I think we should wait to hear back from @fabianfett, since he rolled back the "old version files" style from https://github.com/swift-server/swift-aws-lambda-runtime when we added 5.6 support there, and I cannot remember why - only that some scenario was not working for him.

@tomerd
Copy link
Member

tomerd commented Aug 5, 2022

@yim-lee @ktoso @Lukasa until we hear more back from @fabianfett, let go with the "old files style", i.e make Package.swift required 5.6 and add files for each one of the old version we support.

@yim-lee yim-lee merged commit c98d70e into apple:main Aug 5, 2022
@yim-lee yim-lee deleted the docc branch August 5, 2022 19:57
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.

4 participants