Add "Run piped external commands" example #297

ludwigpacifici · Sep 26, 2017

For a given , shows the <number> biggest
files/subdirectories and its human readable size.

Equivalant Unix command:
du -ah | sort -hr | head -n

budziq · Sep 27, 2017

budziq requested changes Sep 27, 2017

View changes

Hi @ludwigpacifici
Thanks for the PR! 👍 I have few suggestions below to pare it down a little. Overall we should be able to reduce the number of lines significantly.

Also, could you rebase the example? It currently is not able to be build due to conflict (the merge commit conflicts with current master)

Please ping me If you have any questions or need help!

budziq · Sep 27, 2017

src/basics.md

+<a name="ex-run-piped-external-commands"></a>
+## Run piped external commands
+
+[![clap-badge]][clap] [![cat-os-badge]][cat-os]


No need to use clap here.

We would like to make the recipes as short and concise as possible (small snippets that are to the point without additional noise, ready to cut-n-paste into the readers code)

Clap usage makes the example quite noisy. Lets just hardcode the results and directory

budziq · Sep 27, 2017

src/basics.md

+run: `du -ah <directory> | sort -hr | head -n <N>`.
+
+```rust,no_run
+# extern crate clap;


lets remove the clap dependency.

On the other hand we will use error-chain here to handle all the verbose error boilerplate handling. Please checkout
our note about error handling

budziq · Sep 27, 2017

src/basics.md

+    }
+}
+
+# fn declare_command_line_arguments<'a>() -> ArgMatches<'a> {


please remove all the clap related functions. We will not need these

budziq · Sep 27, 2017

src/basics.md

+use std::fs::metadata;
+use std::process::{exit, Command, Stdio};
+
+static DU: &'static str = "du";


I'd remove these constants. The example will be shorter and more readable if we write the tool name and argument list directly in the Command::new builder chain

budziq · Sep 27, 2017

src/basics.md

+
+fn main() {
+    let arguments = declare_command_line_arguments();
+    let results = parse_number_of_results(&arguments);


i would rename the results variable to more descriptive name such as max_results

budziq · Sep 27, 2017

src/basics.md

+# extern crate clap;
+
+# use clap::{App, Arg, ArgMatches};
+use std::error::Error;


One we finish, the only use that will be needed here will be use std::process::{Command, Stdio};

budziq · Sep 27, 2017

src/basics.md

+        .stdout(Stdio::piped())
+        .spawn()
+    {
+        Err(why) => {


ditto on error handling

budziq · Sep 27, 2017

src/basics.md

+        Ok(process) => process,
+    };
+
+    let sort_stdout = sort_process.stdout.unwrap_or_else(|| {


ditto on error handling and we do not need a separate variable binding. please see my comment on head_output below

budziq · Sep 27, 2017

src/basics.md

+    });
+
+    let head_process = match Command::new(HEAD)
+        .arg(HEAD_ARGUMENTS)


ditto on error handling

budziq · Sep 27, 2017

src/basics.md

+        Ok(process) => process,
+    };
+
+    let head_output = head_process.wait_with_output().unwrap_or_else(|err| {


No need fo the error boilerplate here and we do not need separate variable binding here. We can use

let head_output = Command::new("head") .arg("-n") ... .spawn()? .wait_with_output()?;

ludwigpacifici · Sep 30, 2017

Thanks for your detailed review! I agree with your comments ; the code is shorter and to the point. Let me know what you think.

budziq · Sep 30, 2017

budziq requested changes Sep 30, 2017

View changes

@ludwigpacifici just few final thoughts 👍

budziq · Sep 30, 2017

src/basics.md

+# }
+#
+fn run() -> Result<()> {
+    let directory = ".";


we might use https://doc.rust-lang.org/std/env/fn.current_dir.html here instead

budziq · Sep 30, 2017

src/basics.md

+#
+fn run() -> Result<()> {
+    let directory = ".";
+    let du_process = Command::new("du")


We might be tempted to further DRY the code by extracting the builder into a separate fn like

fn spawn_piped<P, I, A, IN>(program: P, args: I, input: IN) -> Result<Child> where P: AsRef<OsStr>, I: IntoIterator<Item = A>, A: AsRef<OsStr>, IN: Into<Stdio>, { ... }

But I'm not sure if this does not make the code harder to read. I'll let you decide here

I gave it a try:

pipe-process.rs

pipe-process-factorized.rs

I prefer pipe-process.rs:

The goal is to demonstrate pipe processes, however, the spawn_piped function tends to bury it.

If the targeted reader is a beginner, spawn_piped introduce concepts of iterators.

spawn_piped will DRY the code if more processes would be spawn.

If I miss something, let me know, otherwise I will skip spawn_piped.

budziq · Sep 30, 2017

src/basics.md

+        .wait_with_output()?;
+
+    println!(
+        "Top {} biggest files and directories in '{}':\n{}",


I guess that w can hardcode the "10" and lose the variable alltoheter

budziq · Sep 30, 2017

src/basics.md

+
+Shows up to the 10<sup>th</sup> biggest files and subdirectories in
+the specified the current working directory. It spawns three Unix
+processes as external [`Command`]. It is equivalant to run: `du -ah . | sort -hr | head -n


I would mention the Stdio::piped in the description as it's critical here

budziq · Sep 30, 2017

Also could you fix the merge conflict and squash the commits into one?

ludwigpacifici · Oct 1, 2017

Knowing that my commits are already pushed, can I squash and rebase them in this PR or should I create a new PR?

budziq · Oct 1, 2017

budziq requested changes Oct 1, 2017

View changes

nice! just some final cleanup and we are ready to merge! @ludwigpacifici

budziq · Oct 1, 2017

src/basics.md

+        .ok_or_else(|| "Could not capture `sort` standard output.")?;
+
+    let head_output = Command::new("head")
+        .arg("-n")


you can also use args(&["-n", "10"])

budziq · Oct 1, 2017

src/basics.md

 [`seek`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.seek
-[`digest::Context`]: https://docs.rs/ring/*/ring/digest/struct.Context.html
-[`digest::Digest`]: https://docs.rs/ring/*/ring/digest/struct.Digest.html
 [rand-distributions]: https://doc.rust-lang.org/rand/rand/distributions/index.html
 [replacement string syntax]: https://docs.rs/regex/0.2.2/regex/struct.Regex.html#replacement-string-syntax


I don't understand this one. Are you referring to the below code, as a follow up from your above comment?

.arg("-ah") .arg(&directory)

@ludwigpacifici
Well that was not really descriptive on my part 😞 (I was doing 3x reviews concurrently at that time, teaches me not to multitask sorry!).

What I mean was:
Please split this commit into one editing the example and second one editing the links
and the "same here" was about making the regex links use wildcard "*" instead of "0.2.2" which happens in two places

No worries @budziq, you do a good job 👍

budziq · Oct 1, 2017

in regard to the suggested refactoring, I would not be hesitant to introduce iterators as these are already present very early in other examples (please think about cookbook as an appendix to RBE, you know the basics and would like to have an inkling how to solve your problem). I agree that the spawn_piped function might dilute the example somewhat, as discussed I'll leave the decision to you.

as for the refactoring proposed by you you do not have to iterate in a for loop, the Command::args takes an iterator directly in a single call.

budziq · Oct 1, 2017

Knowing that my commits are already pushed, can I squash and rebase them in this PR or should I create a new PR?

Sure you can! You modify the history of your local branch (rebase/squash and whatnot) and just git push --force to you remote, this will automatically update the PR. No need to close/reopen new one 👍

ludwigpacifici · Oct 2, 2017

Thank you for the advice 👍

ludwigpacifici · Oct 3, 2017

Commits splitted. Any other change left to do?

budziq · Oct 3, 2017

Any other change left to do?

I would prefer for the links to be sorted case insensitively. otherwise we are good to merge once travis finishes :)

ludwigpacifici · Oct 4, 2017

@budziq I tweaked my editor to change the sort.

budziq · Oct 4, 2017

@ludwigpacifici Nicely done!

ludwigpacifici · Oct 4, 2017

Thanks for your reviews @budziq !

budziq · Oct 4, 2017

No Problem. Keep them comming 👍
You can see your example already deployed

ludwigpacifici referenced this pull request Sep 26, 2017
Closed
Add "Run piped external commands" example #269

Add "Run piped external commands"
Issue: #269

Verified

This commit was signed with a verified signature.

ludwigpacifici Ludwig PACIFICI

GPG key ID: D8398FA1E6232416 Learn about signing commits

7ed0703

Minor fixes
* Sort links * regex links use wildcard "*" * remove trailing whitespace

Verified

This commit was signed with a verified signature.

ludwigpacifici Ludwig PACIFICI

GPG key ID: D8398FA1E6232416 Learn about signing commits

Loading status checks…

80a9892

budziq merged commit 6ffdf8e into rust-lang-nursery:master Oct 4, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rust-lang-nursery/rust-cookbook

Join GitHub today

Add "Run piped external commands" example #297

Conversation

ludwigpacifici commented Sep 26, 2017

ludwigpacifici referenced this pull request Sep 26, 2017

budziq requested changes Sep 27, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ludwigpacifici commented Sep 30, 2017

budziq requested changes Sep 30, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq commented Sep 30, 2017

This comment has been minimized.

ludwigpacifici commented Oct 1, 2017

budziq requested changes Oct 1, 2017 View changes

budziq left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ludwigpacifici Oct 2, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

budziq commented Oct 1, 2017

This comment has been minimized.

budziq commented Oct 1, 2017 • edited

This comment has been minimized.

ludwigpacifici commented Oct 2, 2017

This comment has been minimized.

ludwigpacifici commented Oct 3, 2017

This comment has been minimized.

budziq commented Oct 3, 2017

This comment has been minimized.

ludwigpacifici commented Oct 4, 2017

Hide details View details budziq merged commit 6ffdf8e into rust-lang-nursery:master Oct 4, 2017 2 checks passed

2 checks passed

This comment has been minimized.

budziq commented Oct 4, 2017

This comment has been minimized.

ludwigpacifici commented Oct 4, 2017

This comment has been minimized.

budziq commented Oct 4, 2017

budziq requested changes Sep 27, 2017

View changes

budziq requested changes Sep 30, 2017

View changes

budziq requested changes Oct 1, 2017

View changes

ludwigpacifici Oct 2, 2017 •

edited

budziq commented Oct 1, 2017 •

edited

budziq merged commit `6ffdf8e` into rust-lang-nursery:master Oct 4, 2017
2 checks passed