Skip to content

Conversation

@ludwigpacifici
Copy link
Contributor

fixes #339

Hello @budziq ! Another PR :-) I also added a commit to update the std badge version.

Copy link
Contributor

@budziq budziq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ludwigpacifici !

Some suggestions below

src/basics.md Outdated
println!("{}", line);
}
}
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Ok(()) might be a boilerplate but lets not hide it as it is necessary to compile the example when copy pasted

src/basics.md Outdated
| [Run an external command passing it stdin and check for an error code][ex-parse-subprocess-input] | [![regex-badge]][regex] | [![cat-os-badge]][cat-os] [![cat-text-processing-badge]][cat-text-processing] |
| [Run piped external commands][ex-run-piped-external-commands] | [![std-badge]][std] | [![cat-os-badge]][cat-os] |
| [Redirect both stdout and stderr of child process to the same file][ex-redirect-stdout-stderr-same-file] | [![std-badge]][std] | [![cat-os-badge]][cat-os] |
| [Continuously process child process' outputs][ex-continuous-process-output] | [![std-badge]][std] | [![cat-os-badge]][cat-os] |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm text-processing badge might make sense. What are your thoughts @ludwigpacifici ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Plus it will be homogeneous with similar recipes like Run an external command and process stdout.

src/basics.md Outdated
let reader = BufReader::new(stdout);
for line in reader.lines() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we try to express this in a functional style with filter_map filter and for_each?

src/basics.md Outdated
# extern crate error_chain;
#
use std::process::{Command, Stdio};
use std::io::BufReader;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets group these imports

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I remember rustfmt was doing this for me before. But not now. Anyway, let's group them.

src/basics.md Outdated
stdout](#ex-parse-subprocess-output), while an external [`Command`] is
running, process its standard output. A new pipe is created by
[`Stdio::piped`] and it is read by a [`BufReader`]. Note, a buffered
reader uses a buffer to reduce the number of I/O requests.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we need to explain what BufReader does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it's slightly off topic. While I was implementing the example, I ask myself the question std::io::BufReader vs std::io::Reader. I will remove it. Plus the BufReader link to the doc explains it:

It can be excessively inefficient to work directly with a Read instance. For example, every call to read on TcpStream results in a system call. A BufReader performs large, infrequent reads on the underlying Read and maintains an in-memory buffer of the results.

src/basics.md Outdated

Contrary to [Run an external command and process
stdout](#ex-parse-subprocess-output), while an external [`Command`] is
running, process its standard output. A new pipe is created by
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the explanation is slightly unclear. I'd emphasize that the we do not wait for the child process to finish and process the output continuously.

@budziq budziq merged commit 8543f72 into rust-lang-nursery:master Dec 1, 2017
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.

Add "Process child process output continuously" example

2 participants