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

Better error handling #4

Closed
isomorpheme opened this issue Feb 25, 2016 · 6 comments
Closed

Better error handling #4

isomorpheme opened this issue Feb 25, 2016 · 6 comments

Comments

@isomorpheme
Copy link

Right now, a lot of errors are returned using Option. I think it's more idiomatic and user friendly if errors are returned using Result, because then consumers of this library can use the error information to do their own error reporting, or handle an error gracefully. Currently the code handling errors just does hardcoded println!s, which is only unwanted output IMO, and causes information loss. It should be pretty simple to change uses of Option<T> to Result<T, &str> or something similar, and I'm willing to make a pull request for it.

@jhasse
Copy link
Owner

jhasse commented Feb 25, 2016

Fully agree :) Thanks for working on this.

@vimalloc
Copy link

+1

If I can find some time in the next couple months I would be happy to help with this as well.

@jhasse
Copy link
Owner

jhasse commented May 13, 2016

I think @ijks already pushed some of his changes here: https://github.com/ijks/ears/tree/better-error-handling

Looking forward to it! :)

@vimalloc
Copy link

Thanks for the link! I'll see if I can coordinate with him :)

@isomorpheme
Copy link
Author

isomorpheme commented Mar 14, 2017

So this fell a bit by the wayside. 😅 I think what I had so far was pretty much done, or at least covered all cases where functions used to panic. So I could just open a PR? It's all Result<T, String> though. It might be nicer to be more idiomatic and use a proper error type (using e.g. error-chain or quick-error). But that's probably best left to another PR.

@jhasse
Copy link
Owner

jhasse commented Mar 29, 2017

Yes, just open a PR :)

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

No branches or pull requests

3 participants