sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

Make error diagnostics for byte count flags (`--skip`, `--length`, `--display-offset`) awesome #98

Closed ErichDonGubler closed 4 years ago

ErichDonGubler commented 4 years ago

This PR adds anyhow to the codebase, using it as the reporting layer for contextually layered error diagnostics, and then applying those capabilities to parse_byte_offset. Below are inlined the contents of the commits, since I believe they'll be important for discussion of the design of work here and in the future.


This adds an error type called ByteCountParseError with thiserror. I opted to implement a separate error type for parse_byte_count rather than using anyhow's small ecosystem of error types there (though they are used in <binary>::run) because it allows pairing diagnostic results with tests, incl. those already written. This can be helpful in preventing regressions in error diagnostics.


Using anyhow with {:?} is deliberate, and significantly changes the structure of potential output of top-level errors. Before, errors that have a source were printed on a single line, with the source omitted. Now, an error with a source will be printed like:

Error: <`Display` impl of error>

Caused by:
    <`Display` impl of `source` error>

If there are multiple source errors in a chain, then it will be of the form:

Error: <`Display` impl of error>

Caused by:
    0: <`Display` impl of first `source` error>
    1: <`Display` impl of `source` of `source` error>
    # etc.

Now, in practice this never happened before, since the only error type that ever could return from <binary>::run was std::io::Error, which has no source. However, sources can start being added if, say, future work involved using anyhow::Context::context or custom errors that implement std::Error::source are added to the codebase. I believe this is a good choice for error reporting going forward, but I definitely want to foster discussion and acceptance for it in the open first!

sharkdp commented 4 years ago

The code could be significantly compressed with a dependency like thiserror.

If you are up to it, I'd love to see this. I'm also fine with breaking backwards compatibility (concerning the returned error type). First, it's not like we have tons of reverse dependencies (https://crates.io/crates/hexyl/reverse_dependencies) :smile: and second, I personally never opted into maintaining hexyl as a library. I am, somewhat selfishly, mostly interested in hexyl as a standalone application.

Thank you very much for this contribution. I will look at the rest of the code soon.

sharkdp commented 4 years ago

Found by playing with this: 0xf was previously accepted as a byte count. It is not accepted anymore:

Error: failed to parse `--length` arg "0xf" as byte count

Caused by:
    invalid byte count: no character data found, did you forget to write it?

Can we add a test case for this?

ErichDonGubler commented 4 years ago

Found by playing with this: 0xf was previously accepted as a byte count. It is not accepted anymore:

Oof, regression is embarassing, and lack of test case is embarassing. Fixed.

ErichDonGubler commented 4 years ago

If you are up to it, I'd love to see this. I'm also fine with breaking backwards compatibility (concerning the returned error type). First, it's not like we have tons of reverse dependencies (crates.io/crates/hexyl/reverse_dependencies) 😄 and second, I personally never opted into maintaining hexyl as a library. I am, somewhat selfishly, mostly interested in hexyl as a standalone application.

thiserror is just a library for deriving std::error::Error and Display nicely, so no type changes will be needed, and therefore no backwards compatibility concerns should come from it.

Side note, I've implemented thiserror usage, though I had to ditch the "invalid byte count: " prefix. derive_more could support this, with the same set of transitive dependencies, but I don't know that it's worth splitting hairs over.

sharkdp commented 4 years ago

Thank you very much!