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

Remove unwrap()'s #61

Closed
ghost opened this issue Apr 8, 2019 · 7 comments
Closed

Remove unwrap()'s #61

ghost opened this issue Apr 8, 2019 · 7 comments

Comments

@ghost
Copy link

ghost commented Apr 8, 2019

We have some places in the code where we call unwrap() (4 to be precise).

Maybe we could replace those with unwrap_or* (default value, actual error, etc, depending on the case) for unwrap()'s happening outside the main() method, and as for the ones happening in the main(), printing a nice error message with the logger + exiting instead of panicking.

This has been suggested in Reddit's "what's everyone working on this week": https://www.reddit.com/r/rust/comments/bapows/whats_everyone_working_on_this_week_152019/ekdt4ls

@svenstaro
Copy link
Owner

Yeah, that'd be a good effort.

@ghost
Copy link
Author

ghost commented Apr 9, 2019

Just one quick question, do you mind having an enum with all miniserve's error kind, or do you want them to be split like right now, aka CompressionErrorKind, UploadErrorKind, etc ? In the latter case, I think I need to implement Fail, Display, etc, for all those enums, so some copy/paste. Having a single enum allows to implement it once for good, but that's 1 quite big enum. Your call :)

PS: there might be a third solution, that would allow to have separate enums, but only 1 implementation, but sadly I can't figure out how...

@svenstaro
Copy link
Owner

Ok so honestly I'm thinking hard about both ways and I think I'm entirely balanced concerning my opinion on the matter. For one, it seems nice to have separate errors for separate parts of the application but then again we might also run into scenarios where an error fits into neither category and that'd be awkward.

Cutting down on boilerplate is always nice, of course.

I'll let you make the call on this.

@ghost
Copy link
Author

ghost commented Apr 9, 2019

The argument about an error fitting into no/several categories is very true, didn't think about that. This will probably be the case with IoError, which can happen during compressing, and during uploading.

@svenstaro
Copy link
Owner

svenstaro commented Apr 18, 2019 via email

@svenstaro
Copy link
Owner

Glad to hear you like it!

@svenstaro
Copy link
Owner

Leaving this open until the very last unwrap() is gone :P

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

1 participant