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

@curiousleo curiousleo commented Feb 10, 2020

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
@curiousleo curiousleo requested review from edolstra and grahamc Feb 10, 2020
@edolstra
Copy link
Member

@edolstra edolstra commented Feb 10, 2020

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.

This comment has been minimized.

@edolstra

edolstra Feb 10, 2020
Member

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

This comment has been minimized.

@curiousleo

curiousleo Feb 10, 2020
Author Contributor

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

@curiousleo curiousleo commented Feb 10, 2020

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 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.