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 comments regarding Exec stream redirection in Pipeline context #69

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rockbmb
Copy link

@rockbmb rockbmb commented Apr 11, 2023

Esteemed maintainer(s): I submit this PR after having run into the described problem while using this crate.
Hopefully, you will also find it useful enough to be merged.

Issue

If the first or/and last Exec of a Pipeline has had their standard streams redirected, executing the pipeline will cause a runtime panic.

Changes

This PR documents this caveat in the Exec and Pipeline structs, and adds two doctests in Pipeline::join() demonstrating the panic.

If the first or/and last `Exec` of a `Pipeline` has had their
standard streams redirected, executing the pipeline will cause a
runtime panic.

This PR documents this, and adds some doctests for the issue described.
@hniksic
Copy link
Owner

hniksic commented Apr 12, 2023

Hi, thanks for working on this, it's really useful to document these panics. Maybe I'd opt for a shorter documentation, though, without the examples.

It might be even better to move the panic to functions that create or modify the pipeline, such as Pipeline::from_exec_iter. That way the issue would be detected earlier, in a more logical place for it to fail, and we could document the panic there.

@rockbmb
Copy link
Author

rockbmb commented Apr 16, 2023

@hniksic thanks for having a look!
I can certainly perform those changes, when I'm done I'll ping again for comment.

@rockbmb
Copy link
Author

rockbmb commented May 10, 2023

@hniksic Hello,
Apologies for the delay; new job.

I have a question:

It might be even better to move the panic to functions that create or modify the pipeline

Does that mean we remove the panic from the impl Exec functions?
IMO it should be left there too, because some users may just be joining Execs and not Pipelines; the panic removal would unexpectedly allow improper behavior for them.

@hniksic
Copy link
Owner

hniksic commented May 10, 2023

You're right of course, good catch!

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