-
Notifications
You must be signed in to change notification settings - Fork 3
feat: remove the anyhow and introduce no_std feature flag
#6
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
base: main
Are you sure you want to change the base?
Conversation
ft-filancore
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested in #5, a few preliminary comments regarding the no_std setup. If you have any questions, I'm happy to help.
| default = ["std", "send-sync-storage"] | ||
| std = ["thiserror/std", "async-trait"] | ||
| send-sync-storage = [] | ||
| no-std = ["thiserror", "async-trait"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a side note: cargo feature flags should be additive. Usually, you don't need a no_std feature flag if your modes of operations include no_std and std. You always include thiserror and async-trait as non-optional dependencies because you need them for no_std anyway. And then you only need the std feature flag which additionally activates their respective std flags.
In this specific case, the reason is more semantically rather than technical because currently enabling both std and no_std wouldn't conflict. Later, they might diverge and break the promise. As you don't cfg(feature = "no_std") anything, it should be fine to remove this.
| #[cfg(feature = "std")] | ||
| use std::error::Error as StdError; | ||
|
|
||
| #[cfg(not(feature = "std"))] | ||
| use core::error::Error as CoreError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #[cfg(feature = "std")] | |
| use std::error::Error as StdError; | |
| #[cfg(not(feature = "std"))] | |
| use core::error::Error as CoreError; | |
| use core::error::Error as CoreError; |
std::error::Error is a reexport of core::error::Error so you don't need to differentiate between them, you can simplify this to always using the core version. Similarly, you can always use alloc::string::String and alloc::boxed::Box and it will be the same types as the std versions if the std library is available.
So in the end, your own Error enum does not even need feature-gated duplication.
| #[cfg(feature = "std")] | ||
| extern crate std; | ||
|
|
||
| #[cfg(not(feature = "std"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you remove this gate and simply always use the alloc types. You could even enable #![forbid(clippy::std_instead_of_alloc, clippy::std_instead_of_core)] to force yourself into using the alloc and the core types over the std types where available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cargo toml lacks the MSRV bump to 1.81 to use core::error::Error.
Description of change
removes the dependency on anyhow and replaces it with a generic Error
introduces the no_std flag, which removes the dependency on the std library
Type of change
How the change has been tested
Change checklist