ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
548 stars 66 forks source link

Add `Process.run ?is_success` to control definition of success #586

Closed SGrondin closed 1 year ago

SGrondin commented 1 year ago

Motivation

Some programs (ab)use exit codes in unconventional ways.

As an example I recently ran into: the npm outdated command checks a project's NPM dependencies and prints a status report (outdated, deprecations, etc).

But the exit code is 1 if any dependencies are in need of an update! While it successfully generated a status report, it still returned 1. Different return codes may indicate "true failures". I disagree with the design of this command, I'm not the only one and it's causing issues.

Regardless, this command is far from alone. Plenty of other programs behave in a similar way. Also, I may want to consider certain exit codes to be errors/success depending on the stdout output of the command, for example.

Suggestion

I suggest adding the following optional argument to Eio.Process.run: ?is_success:(int -> bool)

Since it runs after the process has exited, the user is able to inspect the output of stdout and stderr to make a decision.

But why not just call Eio.Process.spawn instead?

Eio.Process.run is a nicer interface, I wouldn't want to lose on (or have to recreate its) error handling. Most importantly though, Process.spawn is a leaky abstraction while Process.run is not. I don't have to stop and dig into Eio internals to use Process.run, I can quickly invoke it without breaking mental focus.

Adding is_success minimizes the number of use cases that require Process.spawn and (in my opinion) is a nice addition to Process.run that doesn't significantly increase complexity for the users nor maintainers.

PR suggestions > Issue suggestions, am I right? 😄

SGrondin commented 1 year ago

Thanks for the review @talex5 For parse_out do you think it would be better to also add the argument to await_exn (which is exposed in the .mli) or just to parse_out itself?

talex5 commented 1 year ago

For parse_out do you think it would be better to also add the argument to await_exn (which is exposed in the .mli) or just to parse_out itself?

Adding to await_exn (as an optional argument) makes sense to me.

SGrondin commented 1 year ago

All done, feel free to edit the .mli docs/comments, it was difficult to phrase