-
-
Notifications
You must be signed in to change notification settings - Fork 148
use the stacktrace of cause, if that doesn't have one, use the one fr… #215
base: master
Are you sure you want to change the base?
Conversation
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.
This seems like a fairly straightforward improvement. Would it be possible to add a test covering the error case?
oh, right. I just saw that there is a reference I missed in http.go. I'll add that. Or are you talking about something different? |
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 mean test coverage in general- a test proving the change does what it suggests to ensure future changes maintain or improve upon the behavior.
sure, will do. |
This is a high value change. Without this change, as far as I can tell, reporting errors via |
yes, that is exactly what is happening. |
@femaref do you mind pushing your http fix, at least to your repo? I'd like to use your branch instead of creating another one |
will do once I'm back at my notebook. |
@jotto actually, it's part of the refactor branch: https://github.com/femaref/raven-go/blob/refactor/transport.go |
thank you @femaref is there an intent with the refactor beyond addressing the stacktrace issue? |
yes, there is a lot of duplicate code between |
…om err
#215