Skip to content
This repository has been archived by the owner on Aug 30, 2023. It is now read-only.

use the stacktrace of cause, if that doesn't have one, use the one fr… #215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

femaref
Copy link

@femaref femaref commented Oct 29, 2018

…om err

#215

Copy link
Member

@dcramer dcramer left a 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?

@femaref
Copy link
Author

femaref commented Nov 1, 2018

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?

Copy link
Member

@dcramer dcramer left a 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.

@femaref
Copy link
Author

femaref commented Nov 2, 2018

sure, will do.

@jotto
Copy link

jotto commented Nov 17, 2018

This is a high value change.

Without this change, as far as I can tell, reporting errors via raven.CaptureError at the top of a callstack will lose their original pkgErrors.Wrap stacktrace.

@femaref
Copy link
Author

femaref commented Nov 18, 2018

yes, that is exactly what is happening.

@jotto
Copy link

jotto commented Nov 18, 2018

@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

@femaref
Copy link
Author

femaref commented Nov 18, 2018

will do once I'm back at my notebook.

@femaref
Copy link
Author

femaref commented Nov 18, 2018

@jotto actually, it's part of the refactor branch: https://github.com/femaref/raven-go/blob/refactor/transport.go

@jotto
Copy link

jotto commented Nov 18, 2018

thank you @femaref

is there an intent with the refactor beyond addressing the stacktrace issue?

@femaref
Copy link
Author

femaref commented Nov 18, 2018

yes, there is a lot of duplicate code between Capture* and Capture*WithWait. I'm reducing that duplication.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants