Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager or direct message in the Swift forums).
What goes into a review of a proposal?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift.
When reviewing a proposal, here are some questions to consider:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Thank you for helping improve the Swift programming language and ecosystem.
This proposal definitely addresses some pain points our team experiences as we modularise our code base into packages. These pain points currently require us to manually run code generators or try and maintain Xcode projects alongside the packages to work with a tool (eg swift protobuf and mockingbird). Looking forward to not having to do this so we can focus on writing code.
The proposal mentions open questions. Were they resolved? I have no idea how to answer them but I thought it might be worth drawing the community’s attention to them. From the proposal:
Should the whole package graph be available to every kind of plugin, or just the package to which it is being applied? If the whole graph is available, then does the plugin entry point need a separate parameter to specify the package to which the plugin is being applied? Vending the entire package graph to the plugin can cause unintentional dependencies on packages "higher up" in the hierarchy.
I agree with @kiel’s point that it appears the proposal has been put for review without resolving an open question that the proposal states should be determined before review. My feedback would simply be to suggest that the question be resolved for a focused re-review.
The proposed APIs seem reasonable. I have limited expertise with this area but all of this reads pretty well and I particularly like that the intention is to “dog food” the proposed feature.
Two nits:
Given the enum cases spelled pkgConfig and artifactsArchive, it would be most consistent for the enum case to be spelled xcFramework rather than xcframework.
Given the overall Swift convention to omit “with” from argument labels and the proposed tool(named:) API, it would be most consistent to have sourceFiles(suffixed:) rather than sourceFiles(withSuffix:).
The “with” here isn’t vacuous, though; it’s a short form of “that have” or something. “suffixed” wouldn’t be wrong, but it does seem weird, given how uncommon it is to use “suffix” as a verb as opposed to a noun.
Apologies, I did forget to remove this section at the bottom before this was put up for review. Thanks for pointing this out and sorry for the confusion. The proposed approach is that only the package to which the plugin is directly applied (and its dependencies) should be passed to the plugin. For a build tool plugin, this would mean the package that contains the target that specifies usage of the plugin. If this proposal is accepted with amendments, one of them would be to remove this section and to clarify this in the body of the proposal.
That's a good point, and makes sense. I proposed this capitalization because it matches the capitalization that Xcode uses for its filename suffix. But I can see that either this should be capitalized as you suggest or perhaps the other one should be called artifactbundle which is what the suffix is.
For these I used the same enum as in the SwiftPM source code, but didn't consider that that implementation was already a bit inconsistent.