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

Add an ability to read exit code and specific streams #89

Closed
wants to merge 1 commit into from

Conversation

Deniallugo
Copy link

@Deniallugo Deniallugo commented Jul 1, 2024

Description

This PR enhances the functionality of the crate by adding the ability to read both the necessary stream and the exit code of a process.

Motivation

First, I want to commend you on creating such a fantastic crate. It has significantly streamlined my work with processes, saving me a lot of time and effort.

The current implementation assumes that the presence of stderr indicates a non-zero exit code, which is not always accurate. In my application, this distinction is crucial because I need to print stdout directly while only reading stderr. Additionally, not having direct access to the exit status forces me to parse stderr for all possible scenarios, making the process error-prone and inefficient.

Check status doesn't fulfill my needs, because I'm either getting the exit status or stderr. And I'd like to have both

Changes

Added functionality to read the exit code of a process altogether with the specified stream.

Signed-off-by: Danil <deniallugo@gmail.com>
@matklad
Copy link
Owner

matklad commented Jul 1, 2024

🤔 I think the idea is that this use-case should be handled by the fn output method, which returns an std::process::Output -- the most low-level and flexible interface.

So, I'd rather not add an extra StreamOutput type, and keep only two levels of abstraction:

  • I want to treat non-zero code as an error and want to get the output as a string -- fn read
  • I want to manually handle all streams, decoding, and exit code -- fn output

Though, as it stands I think fn output doesn't allow the logic of "inherit stdout, but capture stderr", it always captures both.


In terms of actionable things:

  • What you could do right now is to use From<&Cmd> for std::process::Command impl to get the std::Command which provides you with all customizability.
  • Adding StreamOutput is not something we should be doing, we should enable Output
  • Extending the API to make fn output selectively inherit streams sounds good to me, as long as it preserves backbards compatability. After DRAFT: Simplify the API and inner implementation #85 is merged (cc @elichai), I'd love to take a holistic look at how we can simplify the API overall. I think this PR illustrates a good end-goal: "API that gets you std::process::Output while allowing fine-grained customization".

@Deniallugo
Copy link
Author

Great, Thank you! I'll try this direction!

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