Skip to content

Error type #10

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

Merged
merged 1 commit into from
Oct 28, 2015
Merged

Error type #10

merged 1 commit into from
Oct 28, 2015

Conversation

AerialX
Copy link
Contributor

@AerialX AerialX commented Oct 26, 2015

Thoughts/comments on this approach?

@posborne
Copy link
Member

Thanks for the PR @AerialX. Initially, I like this approach. I'll take a deeper look later tonight.

/// Interface to an I2C Slave Device from an I2C Master
///
/// Typical implementations will store state with references to the bus
/// in use and the address of the slave device. The trait is based on the
/// Linux i2cdev interface.
pub trait I2CDevice {
type Error;
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if there should be any additional traits required on this type. Nothing strikes me as really being worth requiring, so I think this is fine.

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 tried requiring std::error::Error but then found out nix::Error didn't impl it. I plan on addressing that in its own PR. fmt::Debug wouldn't be too bad to require, since Result::unwrap needs it.

Copy link
Member

Choose a reason for hiding this comment

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

The other thing that could have an impact is if we want to support no_std so devices could be used, for instance, in something like zinc. In that case, traits out of std (such as Debug) pose a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair! Debug is in core though, only Error is in std (and it really shouldn't be, but...)

@posborne
Copy link
Member

Overall, I think I like this. Do you know of any other projects making use of this pattern in a similar fashion? I searched around rust itself and found some instances that are somewhat similar but not quite the same. I would be curious to know if this approach has been taken with some success in the past?

@AerialX
Copy link
Contributor Author

AerialX commented Oct 28, 2015

I'm under the impression than an Error associated type is pretty standard, I'd probably need to dig to find real examples though. The alternatives aren't great: enums that try to predict what's needed, io::Error or boxed Error trait objects, etc. Ensuring that the type impls error::Error and perhaps Into<io::Error> is usually a good way to ensure it meshes well into other libraries and code.

@posborne
Copy link
Member

Ok, this looks like a great start. I'm going to merge and we can consider adding additional requirements on the Error types in other PRs.

posborne added a commit that referenced this pull request Oct 28, 2015
@posborne posborne merged commit ff38069 into rust-embedded:master Oct 28, 2015
@posborne
Copy link
Member

Noticed that upstream implementation of Error has landed in nix upstream: nix-rust/nix#199

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.

2 participants