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

impl std::error::Error for cargo scout Error #58

Closed
o0Ignition0o opened this issue Dec 18, 2019 · 5 comments · Fixed by #73
Closed

impl std::error::Error for cargo scout Error #58

o0Ignition0o opened this issue Dec 18, 2019 · 5 comments · Fixed by #73
Assignees
Labels
help wanted Extra attention is needed

Comments

@o0Ignition0o
Copy link
Owner

As mentionned in this comment cargo scout Error is expected to implement std::error::Error.

This is something that would probably only require us to add an impl Display to it :)

@o0Ignition0o o0Ignition0o added the help wanted Extra attention is needed label Dec 18, 2019
@undef1nd
Copy link
Contributor

Plz, assign this one to me. Thanks!

@o0Ignition0o
Copy link
Owner Author

Yay!
Let me know if you have any question ! 🎉

@o0Ignition0o
Copy link
Owner Author

I've had a bit of a look at this issue, and here's something pretty cool!

As explained in the Error trait description Errors must describe themselves through the Display and Debug traits, and may provide cause chain information., so it means it could be implemented automagically if we add an impl Display for Error:

impl Display for Error {
    fn fmt(&self, f: &mut Formatter<'_>) -> Result {
        // our impl happens here 
    }
}
// The magic happens here
impl std::error::Error for Error {}

We also want to have the std error trait in scope, but not callable (it would cause conflicts with our Error enum) eg:

// We want the trait to be in scope,
// But we don't want it to cause conflicts
// with the enum name.
use std::error::Error as _;

One drawback of it is that it won't implement the cause() function on its own, so we might want to provide it ourselves.

So this issue has actually two parts:

  • Implement Display for Error
  • Implement std::error::Error for Error (with a cause() function)

Both steps can be split in separate issues, or tackled in this one, depending on how hard it would be, and how long it would take (and if you wanna do it of course :D)

Please let me know what you think 🎉

@ebroto
Copy link
Contributor

ebroto commented Dec 27, 2019

Just to add my 2 cents, I think we should implement source instead of cause as the latter has been deprecated.

We could also think about using the great thiserror to reduce the boilerplate of the From implementations, getting also the source implementation for free, though we should think if we want to add a dependency for this :)

@o0Ignition0o
Copy link
Owner Author

Oh that’s an excellent crate !
It would even solve the code coverage 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants