-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add logging #84
Add logging #84
Conversation
cc @fho |
I decided to go with the simplest solution and just use the |
Why is logging the errors the right approach to handle them? If one of those operations fail and cause e.g that a connection becomes unusable this must be either smoothly handled by the package or the error must be forwarded to the user, so the user can react and close the connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second most (if not all) of the review of @fho
I didn't want to change the API of the project at this time. But, maybe I shouldn't worry so much about it. Thanks @fho and @ansd. I suspect I'll be able to remove the need for logging most or all of these errors by changing the API to ensure errors are returned when appropriate. |
973df1e
to
4143243
Compare
@fho @ansd OK it turns out that the cases where we were ignoring errors they can in fact be ignored. However, I still would like the ability to turn on logging just in case someone is having a weird issue and logging these places could assist. Plus, this gives us a starting place should we wish to add very detailed debug logging at some point. By default logging is not enabled, though it can be via |
4143243
to
722bea6
Compare
Correctly exit test on errors Get rid of a couple more panics Replace nolint / TODO with logging Use single Logger Add more informative log messages
8818ca6
to
4f701fd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, thank you!
Thanks @fho we really appreciate the time you take with this project. |
Fixes #81