Switch from clap to quicli #2

mistydemeo · 2018-02-19T07:00:38Z

Clap is great, but it's a bit verbose. I'm liking the combo of StructOpt with quicli's main! macro.

killercup

Cool!

Coming here from killercup/quicli#66 I just wanted to leave some comments on the error handling :)

This assumes you use quicli's Result alias or a Result<_, ::failure::Error> (Error is also re-exported by quicli IIRC) in the functions where you return FontCreationError right now.

killercup · 2018-02-19T07:28:07Z

src/errors.rs

+use std::error::Error;
+
+#[derive(Debug)]
+pub struct FontCreationError {


You might want to use the failure crate and derive Fail :)

killercup · 2018-02-19T07:28:42Z

src/main.rs

    if !input_dir.exists() {
-        return Err(format!("Directory does not exist: {}", input_dir.to_string_lossy()));
+        return Err(FontCreationError::new(format!("Directory does not exist: {}", input_dir.to_string_lossy())));
    }


This can be rewritten as

ensure!(input_dir.exists(), "Directory does not exist: {}", input_dir.to_string_lossy());

Where are the ensure! and bail! macros defined? I wasn't able to find their documentation when I was looking for them.

Failure exports them: https://docs.rs/failure/0.1.1/failure/index.html#macros

killercup · 2018-02-19T07:30:26Z

src/main.rs

    }
    match glob(&input_dir.join("*.png").to_string_lossy()) {
        Ok(glob) => return Ok(glob),
-        Err(e) => return Err(format!("Error listing files: {}", e)),
+        Err(e) => return Err(FontCreationError::new(format!("Error listing files: {}", e))),
    }


I'd rewrite that like

glob(&input_dir.join("*.png").to_string_lossy()) .with_context(|e| format!("Error listing files: {}", e))

Does this also require using failure types? Attempting to use this nets the following error:

--> src/main.rs:52:5 | 48 | fn list_tiles(input_dir : &PathBuf) -> Result<Paths, FontCreationError> { | -------------------------------- expected `std::result::Result<glob::Paths, sss_fontbuild::errors::FontCreationError>` because of return type ... 52 | / glob(&input_dir.join("*.png").to_string_lossy()) 53 | | .with_context(|e| format!("Error listing files: {}", e)) | |________________________________________________________________^ expected struct `sss_fontbuild::errors::FontCreationError`, found struct `quicli::prelude::Context` | = note: expected type `std::result::Result<_, sss_fontbuild::errors::FontCreationError>` found type `std::result::Result<_, quicli::prelude::Context<std::string::String>>`

Actually, it looks like I'm still seeing a variant on this even when I'm using an error type that implements Fail.

The concrete error type here is quicli::prelude::Context<std::string::String> (feel free to read quicli::prelude:: as failure::), so you need a return error type that has a From<Context<String>> implementation. failure::Error is a trait object which fulfills that. For you own type, you'd need to add that implementation specifically. Or, you know, just use failure::Error :)

killercup · 2018-02-19T07:30:50Z

src/main.rs

@@ -56,7 +76,7 @@ fn collapse_bits(bytes : &[u8]) -> Result<u8, String> {
            0 => result |= mask,
            1 => result &= !mask,
            _ => {
-                return Err(format!("Bits must be either 0 or 1 (value was {})", *byte));
+                return Err(FontCreationError::new(format!("Bits must be either 0 or 1 (value was {})", *byte)));


bail!("Bits must be either 0 or 1 (value was {})", *byte);

Looks like both this and ensure! are unhappy about the error type:

--> src/main.rs:79:17 | 79 | bail!("Bits must be either 0 or 1 (value was {})", *byte); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `sss_fontbuild::errors::FontCreationError`, found struct `quicli::prelude::Error` | = note: expected type `sss_fontbuild::errors::FontCreationError` found type `quicli::prelude::Error` = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

Is the failure create's Fail trait necessary for this to work?

Yeah, the macro can't know how to construct your error with a string otherwise. (It calls failure's err_msg internally which returns a failure::Error.)

If you wanted, you could probably copypaste these macros into your code quite easily, though.

Actually, I haven't tested if this works with quicli's re-export! You might need to include failure directly.

Actually, I haven't tested if this works with quicli's re-export!

With some quick testing, it looks like that's the case. quicli's re-export doesn't appear to make Fail available. For example, trying to use it nets this error:

error[E0433]: failed to resolve. Maybe a missing `extern crate failure;`? --> src/errors.rs:6:17 | 6 | #[derive(Debug, Fail)] | ^^^^ Maybe a missing `extern crate failure;`?

So it does in fact look like the reexport isn't going to be available without independently using failure.

Please ignore my earlier edit about the conflict - I misread an error message. However, it is the case that it doesn't seem to be possible to use Fail without independently importing both failure and failure_derive.

The code failure_derive 0.1.1 produces seems to insert extern crate failure; but I can't find the code as the crate seems to be undergoing quite a large refactoring. As probably all the comments here already tell you: We need to have a better story integrating failure :)

killercup · 2018-02-19T07:31:43Z

src/main.rs

        Ok(f) => target_file = f,
        Err(e) => {
-            println!("Unable to open target file {}!\n{}", target, e);
+            println!("Unable to open target file {}!\n{}", args.target.to_string_lossy(), e);
            exit(1);


bail!("Unable to open target file {}!\n{}", args.target.to_string_lossy(), e);

(Returning the error exits main! with a non-zero exit code)

mistydemeo · 2018-02-21T04:38:48Z

After experimenting briefly with the failure types, and its macros, I didn't feel like the convenience it brought is worth the reduced readability/googleability for people who aren't familiar with them. Going to go with this version. Thanks!

mistydemeo mentioned this pull request Feb 19, 2018

Move Result type out of the prelude killercup/quicli#66

Open

Swap bare clap for quicli

af7ba39

mistydemeo force-pushed the switch_to_quicli branch from 8f2b790 to e3b334c Compare February 19, 2018 07:11

mistydemeo added 2 commits February 18, 2018 23:24

Use custom error type, not strings

d8bba6c

Use the question mark operator in main

19cceb9

mistydemeo force-pushed the switch_to_quicli branch from e3b334c to 19cceb9 Compare February 19, 2018 07:24

killercup reviewed Feb 19, 2018

View reviewed changes

mistydemeo added 2 commits February 19, 2018 22:00

Move utils out into separate file

6b16b3d

utils: don't need to underscore-prefix anymore

0ea0e5c

mistydemeo mentioned this pull request Feb 20, 2018

Remove failure rexports killercup/quicli#67

Open

mistydemeo merged commit a7dbad3 into master Feb 21, 2018

mistydemeo deleted the switch_to_quicli branch February 21, 2018 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from clap to quicli #2

Switch from clap to quicli #2

mistydemeo commented Feb 19, 2018

killercup left a comment •

edited

killercup Feb 19, 2018

killercup Feb 19, 2018 •

edited

mistydemeo Feb 19, 2018

killercup Feb 19, 2018

killercup Feb 19, 2018

mistydemeo Feb 20, 2018

mistydemeo Feb 20, 2018

killercup Feb 20, 2018

killercup Feb 19, 2018

mistydemeo Feb 19, 2018 •

edited

killercup Feb 19, 2018

killercup Feb 19, 2018

mistydemeo Feb 20, 2018 •

edited

mistydemeo Feb 20, 2018

killercup Feb 20, 2018

killercup Feb 19, 2018

mistydemeo commented Feb 21, 2018

Switch from clap to quicli #2

Switch from clap to quicli #2

Conversation

mistydemeo commented Feb 19, 2018

killercup left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killercup Feb 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistydemeo Feb 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistydemeo Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistydemeo commented Feb 21, 2018

killercup left a comment •

edited

killercup Feb 19, 2018 •

edited

mistydemeo Feb 19, 2018 •

edited

mistydemeo Feb 20, 2018 •

edited