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

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

Merged
merged 4 commits into from
Jun 17, 2020

Conversation

ErichDonGubler
Copy link
Contributor

@ErichDonGubler ErichDonGubler commented Jun 11, 2020

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!

@ErichDonGubler ErichDonGubler force-pushed the better-diagnostics branch 3 times, most recently from f758092 to 944f0ff Compare June 11, 2020 18:52
…r reporting flexibility

This 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, `source`s 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 going forward, but I definitely want to foster
discussion and acceptance for it in the open first!
@ErichDonGubler ErichDonGubler force-pushed the better-diagnostics branch 2 times, most recently from 27a90a4 to 0ab6c96 Compare June 11, 2020 19:04
@sharkdp
Copy link
Owner

sharkdp commented Jun 11, 2020

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) 😄 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.

src/bin/hexyl.rs Outdated Show resolved Hide resolved
@sharkdp
Copy link
Owner

sharkdp commented Jun 11, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

…actually good

This adds an error type called `ByteCountParseError` with a vanilla
`std::error::Error` implementation. The code could be significantly
compressed with a dependency like `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.
@sharkdp sharkdp merged commit 13aec94 into sharkdp:master Jun 17, 2020
@sharkdp
Copy link
Owner

sharkdp commented Jun 17, 2020

Thank you very much!

@ErichDonGubler ErichDonGubler deleted the better-diagnostics branch June 17, 2020 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants