Switch to snafu as error handling library

[0ndorio]

This is my first attempt to replace failure with snafu as failure got officially deprcated. It should finally fix #40. Even if it seems like the CI doesn't like this changeset... Travis CI just stucks and on GHA the macos-latest builder are not able to download snafu_derive...

• https://travis-ci.org/github/0ndorio/bendy/builds/743496259
• https://github.com/0ndorio/bendy/actions/runs/362313469

@casey could you review this? I hadn't much chance to program rust during this year and I assume @thequux is still quite busy. Feel free to nitpick as much as you like.

[casey]

This all looks good to me! It think the only substantive comments were about removing From implementations in favor of using #[snafu(context(false))] on variants.

I am not familiar with using Snafu in combination with anyhow. I was under the impression that they were separate, orthogonal error handling strategies:

With Snafu, you have an error enum with different variants, and snafu helps you map underlying errors into your error type, implements display, and other helpful things like that.

Anyhow, on the other hand, provides a bunch of helpers for basically returning Box<dyn std::error::Error> everywhere, mapping errors too that type, and adding underlying errors.

However, I think I might have been mistaken. Is that not the case? With the code in the current PR, do bendy users still concrete bendy error types that they can match on specific error variants?

[casey]

This can be removed if you add context(false) to the variant.

[casey]

I think it's unrelated to this PR, but is there a reason that there are separate Error and ErrorKind types? Is it for future flexibility in case you want to add things to the Error type?

[0ndorio]

As far as I remember the idea was to come up with an error type that allows us to add additional variants without any concerns about potential compatibility issues. I know this sometimes increases the workload to actualy handle a specifc error.

Do you think its worth to discuss this decision?

[casey]

I usually see this pattern used when you have an type with many variants, but there are fields with are common to all variants, so instead of doing:

enum Foo {
  A { id: u32 },
  B { id: u32 },
  C { id: u32 },
}

You do:

struct Foo {
  id: u32,
  kind: FooKind,
}

enum FooKind {
  A,
  B,
  C,
}

It stood out to me in this case because doesn't seem to be any shared data between variants. I don't think it's done to allow backwards compatible extension with new variants, usually that's done by marking the enum as #[non_exhaustive].

But, if you might add shared data between variants, or if you don't mind a breaking change in the future, then I think this isn't critical to address.

[0ndorio]

I can see how #[non_exhaustive] would allow to simplify everything. I could rewrite the structure but I assume thats something @thequux should decide.

[thequux]

Yeah, I think that #[non_exhaustive] makes more sense here than the ErrorKind structure. IIRC, we used the ErrorKind split because it was recommended in https://rust-lang-nursery.github.io/failure/error-errorkind.html, and we didn't think too closely about it at the time.

[casey]

I think this can get context(false) too.

[casey]

This can be deleted if the variant gets context(false)

[0ndorio]

Thanks for the review :) I am going to address your concerns tomorrow!

I am not familiar with using Snafu in combination with anyhow. I was under the impression that they were separate, orthogonal error handling strategies:

With Snafu, you have an error enum with different variants, and snafu helps you map underlying errors into your error type, implements display, and other helpful things like that.

Anyhow, on the other hand, provides a bunch of helpers for basically returning Box<dyn std::error::Error> everywhere, mapping errors too that type, and adding underlying errors.

AFAIK you are right. The strategy of anyhow is to replace failure::Error as a generic version of "i don't actually care about the specific type of the error" value, while snafu took the place of failure::Fail and failure_derive.

The only reason I introduced anyhow in the example was because I was lazy in that moment and didn't cared about the actual return value of the main function but I assume it would be better to remove it again and replace it with a simple fn main() -> Result<(), Box<dyn std::error::Error>> {} to avoid any confusions. It would also allow to remove anyhow from the list of dev-dependencies.

However, I think I might have been mistaken. Is that not the case? With the code in the current PR, do bendy users still concrete bendy error types that they can match on specific error variants?

This should still be possible as any internal method returns either decoding::Error or encoding::Error and not a generic wrapper like anyhow::Error. Also both ErrorKind variants are still exposed in case it is necessary to access or construct them.

[casey]

The only reason I introduced anyhow in the example was because I was lazy in that moment and didn't cared about the actual return value of the main function but I assume it would be better to remove it again and replace it with a simple fn main() -> {} to avoid any confusions. It would also allow to remove anyhow from the list of dev-dependencies.

Ah, okay, I was confused, I didn't notice that anyhow was being used in an example. I think if this is the only use of anyhow, it's probably better to use Result<(), Box<dyn std::error::Error>> instead, just because if a reader isn't familiar with anyhow, they might think something more exotic was going on.

[thequux]

This PR has been sitting here for a few months now, and I'm generally in favor of the direction that it moves the crate. For that reason, I'm going to merge it.

However, I don't think that it is complete. @casey's comments on places where we could use #[context(false)] to remove some From impls and the discussion on the Error/ErrorKind split (which was largely an artifact of historical limitations in the Rust compiler) suggest that we should spend some more time on refactoring the error handling in Bendy to match current idioms.

I'll work on fixing those remaining issues in the next day or two, so expect some more PRs :-)