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 --stdout option / redirect error messages to stderr #79

Merged
merged 1 commit into from Mar 31, 2017

Conversation

@sairion
Copy link
Contributor

@sairion sairion commented Mar 27, 2017

Hi,

I added an option to take output to stdout. This is my first attempt to write Rust code other than hello world, so I believe it could be not-so-much-idiomatic ;) Please take your time to tell me if there's anything to fix!

Also, I redirected error messages to stderr to prevent error messages to be written to stdout. I needed this option because I wanted to replace svgo with svgcleaner, using stream interface in Node.

src/main.rs Outdated
let in_file = args.value_of("in-file").unwrap();
let out_file = args.value_of("out-file").unwrap();
let in_file = args.value_of("in-file").unwrap();
let out_file = args.value_of("out-file").unwrap_or("");

This comment has been minimized.

@RazrFalcon

RazrFalcon Mar 27, 2017
Owner

I think we should use Option<String> in this case. So .unwrap_or("") is not needed.

src/main.rs Outdated

if cli::get_flag(&args, Key::Quiet) {
return;
if !out_file.is_empty() {

This comment has been minimized.

@RazrFalcon

RazrFalcon Mar 27, 2017
Owner

Since we switched to Option<String> it will became:
if let Some(out_file) = out_file {

@RazrFalcon
Copy link
Owner

@RazrFalcon RazrFalcon commented Mar 27, 2017

Looks good, but there are few problems:

  1. Warnings from svgcleaner, svgdom and svgparser will still be printed to stdout. So we have to update all of them. It's not a problem, just a task. I planned to do this one day. So I should update all the dependencies and only then merge this patch.
  2. stdout usually marked as -, --, -c or --stdout. I don't know if clap supports -/--, but -c should be added.
  3. I use custom help text, so you should update data/help.txt as well.

As for code itself - I've added few comments.

src/main.rs Outdated
@@ -72,7 +73,7 @@ fn main() {

let on_err = || {
// copy original file to destination
if cli::get_flag(&args, Key::CopyOnError) {
if out_file.is_empty() && cli::get_flag(&args, Key::CopyOnError) {

This comment has been minimized.

@RazrFalcon

RazrFalcon Mar 27, 2017
Owner

Is there an error? It should be !out_file.is_empty()?

-out_file.is_empty()
+out_file.is_some()
@sairion
Copy link
Contributor Author

@sairion sairion commented Mar 31, 2017

@RazrFalcon I believe -- option is a bit different with standard output flag? It seems clap (kinda) supports it but I don't think it is applicable here.

I've add -c as positional argument and set --stdout as alias of it. Please take a look! 😄

@@ -207,6 +207,11 @@ pub fn write_buffer(doc: &Document, opt: &WriteOptions, buf: &mut Vec<u8>) {
doc.write_buf_opt(opt, buf);
}

pub fn write_stdout(data: &[u8]) -> Result<(), io::Error> {
io::stdout().write(&data).unwrap();
Ok(())

This comment has been minimized.

@RazrFalcon

RazrFalcon Mar 31, 2017
Owner

You should remove either Ok(()) or unwrap(), because this code has no meaning.

src/main.rs Outdated
if in_file != out_file {
try_msg!(fs::copy(in_file, out_file));
if in_file != out_file.unwrap() {
try_msg!(fs::copy(in_file, out_file.unwrap()));

This comment has been minimized.

@RazrFalcon

RazrFalcon Mar 31, 2017
Owner

Too much unwrap. You can use if let syntax here too.

if let Some(out_file) = out_file {
    if in_file != out_file {
        try_msg!(fs::copy(in_file, out_file));
    }
}
@RazrFalcon
Copy link
Owner

@RazrFalcon RazrFalcon commented Mar 31, 2017

I've added two comments. You can fix this issues and squash commits into one. Then I will merge it.

@RazrFalcon
Copy link
Owner

@RazrFalcon RazrFalcon commented Mar 31, 2017

svgdom and svgparser will print warnings to stderr now.

- Add -c flag and --stdout option as alias of it
- print error messages to stderr
@sairion sairion force-pushed the sairion:master branch 2 times, most recently from eef1fcd to 4b7502f Mar 31, 2017
@sairion
Copy link
Contributor Author

@sairion sairion commented Mar 31, 2017

@RazrFalcon fixed, and squashed commits into one (I didn't see your commit mixed in, sorry)

@RazrFalcon
Copy link
Owner

@RazrFalcon RazrFalcon commented Mar 31, 2017

Good. Thanks.

@RazrFalcon RazrFalcon merged commit 1d180c6 into RazrFalcon:master Mar 31, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants