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 calls to unwrap #7

Open
vbrandl opened this issue Apr 8, 2019 · 4 comments · Fixed by #10
Open

Remove calls to unwrap #7

vbrandl opened this issue Apr 8, 2019 · 4 comments · Fixed by #10
Labels
help wanted Extra attention is needed

Comments

@vbrandl
Copy link
Contributor

vbrandl commented Apr 8, 2019

(From my comment on reddit and as a reminder for myself)
You might want to change the notify method to return a Result<(), SomeErrorType> and don't call unwrap() in your code. It is considered bad practice for a library to panic.

@frewsxcv frewsxcv added the help wanted Extra attention is needed label Apr 11, 2019
@frewsxcv
Copy link
Owner

I started working on this (here's the branch), but a little blocked on moving forward because the winrt crate errors don't implement Error

@vbrandl
Copy link
Contributor Author

vbrandl commented Apr 16, 2019

I missed your comment here and implemented it already.

I implemented a new Error type for rust-notifica, which wraps the external error types. I could not test it on Windows, but maybe you can.

@frewsxcv frewsxcv reopened this Apr 28, 2019
@frewsxcv
Copy link
Owner

reopening – we still have a few unwrap calls we need to remove

@Absolucy
Copy link

These two unwraps WILL panic on macOS if notify is used more than once: https://github.com/frewsxcv/rust-notifica/blob/master/src/lib.rs#L177-L178

set_application should be run within a std::sync::Once.

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