Respect TypeModifierOmitStackTrace in Builder #43
Conversation
builder_test.go
Outdated
| err := NewErrorBuilder(et). | ||
| WithCause(testType.NewWithNoMessage()). | ||
| Create() | ||
| require.Nil(t, err.stackTrace) |
There was a problem hiding this comment.
This particular case troubles me. I'm not entirely sure how the case of 'no-trace type is used to wrap whatever other error' is used.
On the one hand, such types are mostly used without wrapping other errors - so, if you do wrap, maybe that's exactly why you added the wrapping type, and you only use it this way. And you expect to avoid dragging any nested stack traces along.
On the other hand, such logic loses the original information. You already paid for its collection, and it may be a nasty surprise for user.
Maybe a better path would be not to enhance stack trace, but not to lose the existing one either?
There was a problem hiding this comment.
On the other hand, such logic loses the original information.
We don't lose anything here. The stack trace in cause is still there. Give me a moment, I'll verify it in tests.
There was a problem hiding this comment.
It looks like I'm wrong and although technically we don't lose anything, fmt.Sprintf("%+v", err) is not going to print nested stack trace if err doesn't have it's own stack trace. It looks like a bug, IMO.
There was a problem hiding this comment.
OK, it may be my mistake in interpreting the test code. Thank you, explicitly testing that part as well would be perfect.
There was a problem hiding this comment.
It looks like a bug, IMO.
It all depends on observable behaviour. Probably, until this point, this made sense.
There was a problem hiding this comment.
Okay, I'll use less offensive words then 🙂
This behaviour doesn't seem correct taking into account the changes in the current PR. I should either fix my PR or fix the (*Error).Format method. Let me think a little bit...
There was a problem hiding this comment.
@PeterIvanov, I've added a new test to ensure that ErrorBuilder keeps stack trace from cause even if error type explicitly requests stack trace omission.
I had to update testify to get access to Same assertion method.
There was a problem hiding this comment.
I've got zero issues with your wording. My point was that the we should change that if it's needed now - but to consider other use cases possibly affected.
There was a problem hiding this comment.
@PeterIvanov, please, check out new version. I've tried to address the concerns you've highlighted.
There was a problem hiding this comment.
Sorry for the force-push, I had to do it due to rebase. However, I've embraced it as the opportunity to glue all my commits into one 🙂
g7r
force-pushed
the
always-respect-omit-stack-trace-in-builder
branch
from
f276d20 to
902010f
Compare
builder.go
Outdated
| ) | ||
|
|
||
| func (eb ErrorBuilder) assembleStackTrace() *stackTrace { | ||
| switch eb.mode { | ||
| case stackTraceCollect: | ||
| return eb.collectOriginalStackTrace() | ||
| case stackTraceBorrow: | ||
| return eb.borrowStackTraceFromCause() | ||
| case stackTraceBorrowOnly, stackTraceBorrowOrCollect: |
There was a problem hiding this comment.
Very minor: perhaps making this into 2 separate cases with a little copy-paste but without another if eb.mode == would improve readability.
Previous implementation of `WithCause` method didn't respect the `TypeModifierOmitStackTrace` modifier. A stack trace was erroneously collected if the error that was passed to `WithCause` didn't have stack trace. I had to upgrade `github.com/stretchr/testify` to a slightly more modern version to be able to use `Same` assertion.
g7r
force-pushed
the
always-respect-omit-stack-trace-in-builder
branch
from
902010f to
b1898dc
Compare
Previous implementation of
WithCausemethod didn't respect theTypeModifierOmitStackTracemodifier. A stack trace was erroneously collected if the error that was passed toWithCausedidn't have stack trace.