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

Replace failure with thiserror #39

Closed
aloucks opened this issue Feb 25, 2020 · 6 comments · Fixed by #40
Closed

Replace failure with thiserror #39

aloucks opened this issue Feb 25, 2020 · 6 comments · Fixed by #40

Comments

@aloucks
Copy link
Contributor

aloucks commented Feb 25, 2020

Error types created by failure do not implement std::error::Error which prevents usage of the ? operator. The thiserror crate provides similar functionality but is fully compatible with the standard library and RFC2504.

I'm happy to submit a PR with the changes but I want collect feedback first.

https://github.com/dtolnay/thiserror

https://github.com/dtolnay/anyhow

@tarcieri
Copy link
Member

I'd be in favor of it. Any thoughts on this @dignifiedquire @str4d ?

@dignifiedquire
Copy link
Member

Yes, fully in favor, I have been switching most of my crates over alrady.

@tarcieri
Copy link
Member

tarcieri commented Feb 26, 2020

Cool, well now that we're in agreement, let me offer the one con of doing it 😅

This crate presently doesn't support no_std, but if the errors use thiserror, it's trickier to make it no_std in the future, since the main benefit of thiserror beyond deriving a std::error::Error impl is having it also write your Display impl.

@dtolnay I don't suppose there's a no_std mode for thiserror I'm not aware of, is there? (i.e. a way to shut off the std::error::Error impl, but still derive Display)? I know it's a bit odd to derive(Error) but not actually impl Error, but it'd be handy to be able to use thiserror's proc macro for the Display impl alone in no_std cases and support the std::error::Error derive if the std feature is enabled.

That said, since this crate doesn't support no_std anyway, I don't see that as a blocker.

aloucks added a commit to aloucks/RSA that referenced this issue Feb 26, 2020
aloucks added a commit to aloucks/RSA that referenced this issue Feb 26, 2020
@dtolnay
Copy link

dtolnay commented Feb 26, 2020

Looking at the PR it seems you mainly need a Display derive, not an Error derive. If you can find a good one of those, no_std support would look like:

#[derive(Display, Debug)]
pub enum Error {
    ...
}

#[cfg(feature = "std")]
impl std::error::Error for Error {}

@tarcieri
Copy link
Member

tarcieri commented Feb 26, 2020

@dtolnay good point! I write the above in a lot of libraries (that is to say, a manual Display impl but the std-feature gated empty impl of std::error::Error), but until you mentioned it I hadn't thought of the simple solution of "just use a good custom derive crate for Display"

Perhaps @yaahc's displaydoc is a good candidate?

@dtolnay
Copy link

dtolnay commented Feb 26, 2020

I am not a fan of that approach with doc comments =/ but it's a choice of personal taste. The implementation is good. ;)

Honestly in your case I don't think a proc macro is called for unless one of your dependencies is already pulling it in; you can easily handwrite the one Display impl.

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 a pull request may close this issue.

4 participants