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

Implement std::error::Error and document error variants #3351

Closed

Conversation

curiousleo
Copy link
Contributor

Rust errors should implement std::error::Error for interoperability.
https://rust-lang.github.io/api-guidelines/interoperability.html#c-good-err

In addition, public modules and definitions should be documented. Since I was looking at this module anyway, I made a start here.
https://rust-lang.github.io/api-guidelines/documentation.html

Before:
Screenshot_2020-02-10 nixrust Error - Rust(1)

After:
Screenshot_2020-02-10 nixrust Error - Rust

Rust errors should implement `std::error::Error` for interoperability.
https://rust-lang.github.io/api-guidelines/interoperability.html#c-good-err

In addition, public modules and definitions should be documented.
https://rust-lang.github.io/api-guidelines/documentation.html
@edolstra
Copy link
Member

This conflicts with some refactoring to Error that I've made (but haven't finished yet) to split it into multiple Error types (and maybe use error_chain or failure, but I haven't decided on that yet).

#[derive(Debug)]
pub enum Error {
/// The store path could not be resolved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this isn't really desirable since it duplicates the error descriptions in the fmt::Display trait implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. The error variants have very explicit names too, so perhaps these docs don't add much.

@curiousleo
Copy link
Contributor Author

This conflicts with some refactoring to Error that I've made (but haven't finished yet) to split it into multiple Error types (and maybe use error_chain or failure, but I haven't decided on that yet).

Okay, good to know! I'll close this PR then.

@curiousleo curiousleo closed this Feb 10, 2020
@Ekleog
Copy link
Member

Ekleog commented Feb 10, 2020

@edolstra FWIW, error-chain is long dead, and failure has been halfway merged into the stdlib -- the part that wasn't is now available in thiserror, which is as a consequence the crate new projects should probably go to by default.

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 this pull request may close these issues.

None yet

3 participants