[Review, 2nd] SF-0007: Subprocess

[Vinicius_Vendramini]

@icharleshu

I agree that Subprocess is still far from being like shell scripting, but that’s very much on purpose. Subprocess is meant to replace NSProcess, which is a low-level API for launching processes. It needs to work for different scenarios, not just scripting, like server-side environments. So, I don’t think Subprocess will ever be able to make shell scripting in Swift much simpler beyond what it offers today -- That should be done by another package or module that focuses solely on scripting so it can make the trade offs that Subprocess can't.

I don't quite understand, why do you believe this should be done by another package? Dave made a good point here (which I hadn't thought about before):

@davedelong

OK, but unless that's a standard module provided as part of the core library, it's unusable in a scripting situation, because you can't import other packages in scripts.

But also, running a simple command and getting its standard output is clearly one of the use cases of Subprocess, so any other packages that do this would end up being either a wrapper for Subprocess or a replacement, which to me is an indication that we should consider adding it to Subprocess itself.

I would also disagree with some comments here that say this would be a simple wrapper that anyone could write in their code. There are a few things that could go wrong or be incomplete in user-defined wrappers, especially when they're using a low-level API like this one. For instance:

@davedelong

Maybe the thrown error could include standardError: CollectedResult and the termination status

I hadn't thought that if we don't collect standardError and include it in the thrown Error, and so there'd be no way of knowing what went wrong in the commands I called. I also don't understand anything about environments, platformOptions, etc, so there are maybe more chances for me to get this wrong. I think we have a good opportunity here to design an API that allows for this common use case while taking important things into consideration in that correct™️ way we expect Apple APIs to do.

That said, I have a feeling that's not what @icharleshu first intended with this review, and I think this could also be added later if we make it clear that's something we're considering. So @icharleshu if you don't want to go through the work of adding this convenience method, how about adding it to the Future Directions section of the review, saying something like "we thought about adding convenience APIs to make it easier to call simple scripts but wanted to seriously consider them without holding up this proposal, so those APIs fall out of scope and may be added by later proposals". The downsides are that it would add some time and bureaucracy (because we'd need a second review) and we'd also need someone who knows what they're doing to actually implement it, but at least this current review could go forward.

[itingliu]

Hi all,

Thank you all for your input. @icharleshu is working on another update. This update will include changes in names and function signatures around input and output, so we're going to do another round of review. I'll start a separate thread for the review when it's ready. Thanks.