[Second Review] SF-0037: Subprocess 1.0

[icharleshu (Charles Hu)]

@johannesweiss

• Is hard to read. Even Subprocess's own codebase seems to prefer to essentially always write Subprocess.run(...) which is of course nicer. So let's do that.

@OneSadCookie

• I always want to call it as Subprocess.run because just run is so vague (and quite foreign to Swift, which has very few global functions).

There's nothing stopping you from writing Subprocess.run, as demonstrated by subprocess’s own testing code. You can always write that if that's what you prefer, so why force everyone to write that when they don't have to?

@johannesweiss

(I do very much disregard all 'having a global run() function makes Swift nice for shell script replacements'.

This is not true. We never said we wanted global run() functions because we "want scripting to look nicer" (that was the argument for having Executable.path for convenience path lookup). We decided to use global functions because:

• Subprocess has a very small and focused purpose: to launch processes with one of the run() functions. We started with Subprocess type and eventually decided to remove it when it became an empty wrapper just to hold static func run() with no instance method, no property, and no initializer. We don't see any benefit in having an empty namespace wrapper.
• Subprocess is NOT in Foundation nor the Standard Library. You do not "accidentally" get these run() functions because someone else imported Foundation nor do you have these run() functions "polluting" the global namespace like global functions from the standard library, such as print.
• You can absolutely write Subprocess.run if you prefer since the module acts like a perfectly fine namespace on its own already.

So far I have not received a convincing technical counterargument around why the points given above are invalid. We've also been using Subprocess ourselves and did not find global run() functions to be an issue so far. Most arguments against global run() function centered around "this is different from usual Swift API design". While I agree with this sentiment, I believe we've given enough technical argument on why we made this "exception".

@johannesweiss

• In this case however it makes dependency injection without wrapping the Subprocess module into a type:

I still maintain the same opinion from the previous review thread:

However, I do not believe it is Subprocess's responsibility to solve that problem. Subprocess has a single, well-scoped job: providing the primitives needed to spawn child processes. It is the client's responsibility to wrap those primitives in whatever abstraction they see fit, asuch as protocol, wrapper type, etc., that suits their testing strategy. This is no different from how consumers of FileManager routinely introduce their own protocol for testing purposes; we would not expect FileManager itself to be a protocol just to accommodate that pattern.

The downstream client should perform wrapping and dependency injection in a way that fits their needs. For example, most clients only use one or two run() functions from Subprocess, so they would only need to wrap the ones they need instead of all six. Swiftly is a great example: it makes extensive use of Subprocess, but it's all funneled through one run().

@OneSadCookie

• too many overloads: basically it means that in any nested code (task groups, closures, etc.) any mistake in the arguments to run results in a generic error

This was one of the feedback items we received from the first review, and it has been addressed in 1.0.0-beta.1. Please see this section of the proposal.