Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return ExitStatus from Cmd::run #93

Closed
wants to merge 1 commit into from

Conversation

domenicquirl
Copy link

Implements #90 (comment): 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
Copy link
Owner

matklad commented Jul 19, 2024

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:

  • My null hypothesis would be that thish should go through From<Cmd> for std::process::Command path --- at the call site, convert a Cmd to std type, and use that to get the full-fidelity API. This perhaps requires more docs, but I think this should be conveninet enough for 10% use-case
  • If it is not, then perhaps we should directly expose all the capabilities of low-level API through existing functions. For example, we could add builder methods to inherit certain streams which would work with existing output method.
  • Finally, perhaps we can add a specific run_and_get_status or some such, but my current gut feeling is that this case is not frequent enough to need a dedicated API

@domenicquirl
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants