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.

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

Successfully merging this pull request may close these issues.

3 participants