-
Notifications
You must be signed in to change notification settings - Fork 5
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
Don't read stderr into memory when not captured #148
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, minor comment about deduplicating the code, but if you can't see a good way to do it I don't think it should hold this PR up.
src/collected_output.rs
Outdated
@@ -54,15 +54,21 @@ impl Waiter { | |||
}); | |||
let mut context_clone = context.clone(); | |||
let capture_stderr = config.capture_stderr; | |||
let stderr_join_handle = thread::spawn(move || -> io::Result<Vec<u8>> { | |||
let mut collected_stderr = Vec::new(); | |||
let stderr_join_handle = thread::spawn(move || -> io::Result<Option<Vec<u8>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could be DRYed up. The code here seems nontrivial, so merging the stdout and stderr capture would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of 4015e1c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, looks good.
Similar to #143. Fixes #26.