Add Process.run ?is_success to control definition of success

[SGrondin]

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? 😄}

[talex5]

Thanks, this looks useful! We should probably do the same for parse_out.

[SGrondin]

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]

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]

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

[talex5]

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

I pushed some minor changes (and also added another test).

Ready to merge once CI passes - thanks!