matklad / xshell

Apache License 2.0
675 stars 28 forks source link

Return `ExitStatus` from `Cmd::run` #93

Closed domenicquirl closed 1 month ago

domenicquirl commented 1 month ago

Implements https://github.com/matklad/xshell/issues/90#issuecomment-2217406011: Cmd::run is made to return the ExitStatus of the underlying command on Ok, allowing access to the exit code on failure if used in conjunction with ignore_status. Also adds a test for this behaviour.

Breaking change since Cmd::run is public API.

matklad commented 1 month ago

I understand why whe need something like this, the rationale in #90 makes total sense to me, but I don't think that's the right way to achieve this.

One thing that run does semantically is that it handles the exist code for you, which is the behavior the user wants in 90% of cases. By returning a status code, we make the remaining 10% possible, but also make the rest of 90% less conventient, as now there's ambiguity of whether the user should be doing something with the result.

And, well, this is also a breaking change which I'd rather avoid.

So, I'd love to explore alternative ways to address the original problem:

domenicquirl commented 1 month ago

Hey, thanks for taking a look!

While I'd hoped having the ExitStatus in all cases would be fine, I can see why you think it's still confusing to have it in the Ok case, where it will always be zero (or, I guess, technically ExitStatus::SUCCESS).

My main issue with the From impl is that it's not very discoverable (I was kinda looking for something just like it, but only learned of it after opening an issue and noticing #89, even though I had been looking through docs.rs) and that there is no documentation on what exactly it does in the conversion. I've opened a separate PR in #94 that exposes the underlying to_command method directly to maybe independently improve the situation here.

I do agree that having an explicit fn run_and_get_status just for this case is very much overkill. I'd love to be able to do what I need while staying within xshell, but I take it that is kinda blocked by #85. So for the time being I guess xshell::cmd!("foo").env("key", "value").to_command().status() isn't too bad, which will default to inherited IO streams.