-
Notifications
You must be signed in to change notification settings - Fork 263
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
Refine API-GUIDELINES recommendations #2832
Conversation
documentation/API-GUIDELINES.md
Outdated
- `Clone/Copy` depends on the size and contents of the structure. They should generally be implemented, unless there is a good reason not to. | ||
- The `Default` configuration needs to make sense for a particular driver, and applying the default configuration must not fail. | ||
- Error types: `Debug/Display`, `PartialEq/Eq`, `Clone/Copy`, `Hash` | ||
- Error types: `Debug`, `PartialEq/Eq`, `Clone/Copy`, `Hash` |
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.
given we raised the MSRV should we ask for core::error::Error
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 noting that Error requires Display
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 think we should impl Error
though. I have a feeling it will help with out documentation related unwrap
and ok
issues. We should be able to make our doc snippet main fn main() -> Result<(), alloc::Box<dyn core::error::Error>>
, and from there we can hopefully ?
everything.
If that required Display
then I guess we found the reason to impl Display
? cc: @jessebraham for your thoughts.
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.
If that required Display then I guess we found the reason to impl Display?
We found the reason to impl Display in this very particular case yes :D
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.
For errors yes it probably makes sense, that's generally pretty straight forward to implement. Still don't think it makes sense for other types though, e.g. config structs or drivers.
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.
Sounds reasonable to me, thanks!
d367b39
to
088d090
Compare
Please don't hesitate to voice your opinion if something still doesn't make sense.