Refactor process spawning with Process.capture#701
Refactor process spawning with Process.capture#701straight-shoota wants to merge 9 commits intocrystal-lang:masterfrom
Process.capture#701Conversation
| # Change the origin in the cache repo to https://foss.heptapod.net/foo/bar | ||
| Dir.cd(library.local_path) do | ||
| run "fossil remote-url -R #{library.name}.fossil https://foss.heptapod.net/foo/bar" | ||
| capture %w[fossil remote-url -R] << "#{library.name}.fossil" << "https://foss.heptapod.net/foo/bar" |
There was a problem hiding this comment.
Until we support %W[] then I think being explicit is more readable:
capture ["fossil", "remote-url", "-R", "#{library.name}.fossil", "..."]The feeling is confirmed for every other cases. An explicit array of strings is leagues better.
There was a problem hiding this comment.
Overall a single array for [cmd, *args] is fine, but Process.new and Process.run shall support the syntax, too.
There was a problem hiding this comment.
I suppose this is a pretty subjective preference. IMO array literal + << reads nicer because there's less noise with quotation marks.
capture %w[fossil remote-url -R] << "#{library.name}.fossil" << "https://foss.heptapod.net/foo/bar"
capture ["fossil", "remote-url", "-R", "#{library.name}.fossil", "https://foss.heptapod.net/foo/bar"]Also, the diff is smaller, and the future diff to %W will also be smaller. So if nothing else, it has patch ergonomics going for it.
| end | ||
| end | ||
|
|
||
| struct Result |
There was a problem hiding this comment.
I'm not sure a nilable out/err is very significant over empty strings, so this Result type doesn't bring anything valuable over {status, output, error} tuple.
There was a problem hiding this comment.
The main reason for a result type over a tuple is that I find it easier to handle. For example, result = Process.capture(...) feels more convenient than status, output, error = Process.capture(...). It's less fragile, can be well documented, and wrapping it in FailedCommandError seems quite useful.
The nilable values are just a convenience feature on top. I believe it might be helpful sometimes to differentiate between whether the output is empty or we didn't even capture any.
There was a problem hiding this comment.
Having a dedicated result type is IMO strictly superior to an implicit tuple type.
It gives full control over it and allows modifications in the future (for example, we could consider making input data such as args and env available).
Ergonomics are also better because we don't rely on implicitly ordered members but named properties.
This is a PoC for refactoring app and spec code to simplify the code for spawning subprocesses and capturing their results as part of the discussions crystal-lang/crystal#16657 and crystal-lang/crystal#7171.
The monkey-patched
Process.captureand.capture_resultmethods are a draft variant.They differ from the proposal in crystal-lang/crystal#16630
Some of the key changes:
Processarguments crystal#14773 (comment)).