search

P3KI / bendy

A rust library for encoding and decoding bencode with enforced cannonicalization rules.
BSD 3-Clause "New" or "Revised" License
77 stars 14 forks source link

Switch to `snafu` as error handling library #50

Closed 0ndorio closed 3 years ago

0ndorio commented 4 years ago

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

@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.

0ndorio commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 :-)