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?
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 would be a struct with static factory functions? Even in that case I think that the return value of those functions would be an enum, it's conceptually a choice among possibilities.
Hisotrically PackageDescription has used a a lot of static factory functions because it wasn't yet possible to annotate enum cases with availability annotations to individual enum cases. Even with that ability, I can see the argument for doing so here, since it allows for default parameter values and the possibility of adding new parameters in future SwiftPM versions.
Yes, structs are generally more flexible, you just usually lose exhaustiveness checking when switching over a value. If that's not how this return value is typically used, a struct would usually be preferred, and then have the two common cases provided as static values or functions.
That makes sense, and is consistent with existing PackageDescription types such as SystemPackageProvider and LibraryType. Those are actually implemented as enums, since there can be only one choice among a set, but they use static functions for creation, as you suggest.
One additional thing that has come up is that for commands that produce output, it would be helpful to allow the user to provide an output path that could be passed to the plugin and which SwiftPM would add to the sandbox to allow the plugin to write to it.
In the first example (DocC), the plugin writes the output to a directory inside its assigned work directory, which is the only place in which it's allowed to modify the file system unless it asks for write access to the package directory. The example has the plugin printing out the path of the directory so the user can access the files.
But this is inconvenient, so the suggestion is to allow an optional --output option the plugin invocation, e.g. swift package generate-docs --output ./my/output/path and to make the main entry point be performCommand(context:, targets:, arguments:, output:) with the last parameter being a Path? depending on whether the user provided a path. The output could be either a file or a directory.
Thanks for trying it out — I wasn't aware of that crash. At this point the bulk of the implementation has been merged behind a feature flag environment variable, so in a nightly toolchain setting SWIFTPM_ENABLE_COMMAND_PLUGINS=1 should let you try this out. This requires a package tools version of 999.0, since it's an experimental feature.
Definitely, and I'm thinking that some way similar to how SwiftArgumentParser allows a command to define arguments would make sense for plugins as well. SwiftPM or the IDE could then query the plugin up-front (in a manner conceptually similar to how SwiftArgumentParser supports emitting shell completions) and then use that information to allow parameters to be specified even in an IDE. I think that should be part of a separate PR, however, to keep this one bounded.
I realize that having a more generalized approach would make a specialized output argument moot (though there are some nuances, such as also having to add this path to the sandbox so that the plugin can write it it), but what I'm suggesting is to add this now and then deal with the more general problem in a follow-on PR.
Another suggestion that has come up in separate conversation is that it would be useful for the plugin to report status. This came up during the pitch as well, but seemed like something to leave for a future proposal. This is yet another area where there is a tradeoff between doing something simple now (e.g. just having a way for the plugin to periodically report percent complete and a message) vs waiting until a much more complete future proposal (e.g. providing nested status with possible attachment files, etc). Are there any strong opinions between providing simple support now that can be replaced later vs waiting to do anything until there is a more complete future proposal?
This seems fine - I worked on a Progress type design for Swift Concurrency a while back but we didn't get to pitch it yet, I think that type would work well with what is proposed here. That call would of course need to be thread-safe but I'm assuming that's a given
Right, this would map fairly naturally to both the CLI and IDEs, I think.
I forgot that NSProgress is actually available in open source Foundation. That would be an alternative, except for this note: In swift-corelibs-foundation, Key Value Observing is not yet available
which makes it tricky to know when to send progress updates to the plugin host. It seems unfortunate to do something different from what's in Foundation, but it's not clear to me how a simple API involving Progress would look in the absence of KVO to notify the plugin host of when it changes.
No, no - we should not just throw in NSProgress here into SwiftPM because we can do the right thing with a progress api that will actually work with swift concurrency (NSProgress is very painful to use there, but possible). We’ll get to it soon enough and hooking it all up does not need much APIs — the API you proposed to add will be ideal to easily integrate either progress api and I think that’s good enough
Makes sense to me. To be clear, some of the APIs assume that the plugin will use Foundation for things like launching subprocesses, but it's a good point that using Foundation types in the API taking things to another level.
Since diagnostics currently expressed through a Diagnostics type that's available to all kinds of plugins, perhaps Progress should be the same, e.g.