Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd "Run an external command passing it stdin and check for an error code" example #310
Conversation
| match String::from_utf8(output.stdout) { | ||
| Err(e) => Err(e.into()), | ||
| Ok(s) => { | ||
| let re = Regex::new(r"[[:punct:]&&[^']]")?; |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
I would avoid using additional crates in this basic example. But this ideas is very neat. How about using it in a separate example for regex. Would you be interested in adding such example? If yea then please add comment with proposed title on regex tracking issue
| { | ||
| let stripped = re.replace_all(&s, " "); | ||
| let mut result = stripped.deref() | ||
| .split_whitespace() |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
in this simplified example we can just split_whitespace not to dilute the point of the snippet.
| let re = Regex::new(r"[[:punct:]&&[^']]")?; | ||
| { | ||
| let stripped = re.replace_all(&s, " "); | ||
| let mut result = stripped.deref() |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
- no need for
muthere once we make the tweaks suggested below. - I would rename the variable to something like
wordsand annotate it with type instead of using turbofish withcollect - no need for deref here
| .map(|s| s.to_lowercase()) | ||
| .collect::<Vec<String>>(); | ||
| result.sort(); | ||
| result.dedup(); |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
no need for the sort dedup. We can collect into a HashSet<_> (please note that the collection item type can be inferred )
| .spawn()?; | ||
| { | ||
| match child.stdin.as_mut() { |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
we actually don't need the surrounding braces as long as we do not really need to use the stdin more than once. We can just
child.stdin.as_mut()?.write_all(b"import this; exit()")As for error handling I would normally use chain_err or map_err if I would like to add more context. But in this example I would try to keep it as simple as possible so just ? might suffice.
| let output = child.wait_with_output()?; | ||
| if output.status.success() { | ||
| match String::from_utf8(output.stdout) { |
This comment has been minimized.
This comment has been minimized.
| use regex::Regex; | ||
| use std::io::Write; | ||
| use std::ops::Deref; |
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
| } else { | ||
| match String::from_utf8(output.stderr) { |
This comment has been minimized.
This comment has been minimized.
budziq
Oct 2, 2017
Collaborator
Instead of this whole match you can just use bail! here. It even takes care of formatting
budziq
requested changes
Oct 5, 2017
|
@oldmanmike nice! Please resolve the merge conflict and two nits below and we are ready to merge! |
| use std::collections::HashSet; | ||
| use std::io::Write; | ||
| use std::process::{Command, Stdio}; | ||
This comment has been minimized.
This comment has been minimized.
budziq
Oct 5, 2017
Collaborator
lets add "#" here to avoid needless newline when example is collapsed
| bail!("External command failed:\n {}", err) | ||
| } | ||
| } | ||
This comment has been minimized.
This comment has been minimized.
budziq
Oct 5, 2017
Collaborator
lets add "#" here to avoid trailing newline when example is collapsed
oldmanmike
added some commits
Oct 5, 2017
budziq
merged commit 79345cc
into
rust-lang-nursery:master
Oct 5, 2017
This comment has been minimized.
This comment has been minimized.
|
Thanks @oldmanmike ! |
oldmanmike commentedOct 1, 2017
•
edited
For issue #268
Relatively new to Rust, so feedback would be amazing.