Closed Richterrettich closed 3 years ago
@drahnr , do you want to take a look? Tell me if you think there are problems with the implementations or if something is missing.
I've removed the generic Other
error type and created String wrapper where necessary. Let me know if you have a better idea how to solve this properly.
I did not want to create public errors that are only created for internal use. This seems fishy that's why I created explicit error types for those.
I've removed the generic
Other
error type and created String wrapper where necessary. Let me know if you have a better idea how to solve this properly. I did not want to create public errors that are only created for internal use. This seems fishy that's why I created explicit error types for those.
They would become public by indirection, when they are the source
of an error that then becomes public. I don't see an issue with the fact that the error variant that is never returned by a function directly.
Other than that, LGTM :+1:
I've restructured the code and removed the error from the test case as you have suggested.
For the PGP package I did a similar thing. The builder is a generated construct from a crate called rust_derive_builder
(this is why my IDE freaks out about the builder too btw.)
The build
method always returns a String error but it is mostly just there for convenience.
I've removed the builder and initialized the struct directly. As a result, there is no more need for an error wrapper.
Do you want to have a final look?
LGTM!
The error type is now an enum with variants for different error scenarios. To reduce boilerplate code the thiserror crate was used. Errors intended for signature handling are more generic because it is likely that library users will implement a custom signer at some point in time.
Fixes #14