Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Destructure Wrap/Wrapf into component operations #76

Merged
merged 7 commits into from Sep 29, 2016
Merged

Conversation

davecheney
Copy link
Member

@davecheney davecheney commented Jul 28, 2016

This PR is a result of the discussions at GopherCon. The focus of the branch is to decompose Wrap (and Wrapf) into their component operations, namely adding a message, errors.WithMessage and adding a stack trace, errors.WithStack which were previously merged into one operation with Wrap.

This is accomplished by adding two top level functions, errors.WithMessage and errors.WithStack and rewriting Wrap and Wrap to work in terms of these operations although a small deviation is present to make sure the recorded stack trace is correct.

The motivation for this change was need to treat each of the following operations as distinct:

  • Adding a context message to an existing error without altering the stack trace.
  • Adding a stack trace to an existing error without the requirement adding and additional message.
  • Retrieving the immediate cause of an error; popping one element of the error stack.

TODO:

  • add tests for WithMessage, WithStack
  • add tests for various permutations of WithMessage, WithStack occurring directly rather than via Wrap/Wrapf.
  • ensure WithMessage properly delegates to its nearest parent Cause that supports StackTrace()

@@ -193,6 +205,18 @@ func Wrapf(err error, format string, args ...interface{}) error {
}
}

// WithMessage annotates err with a new message.
// If err is nil, WithStack returns nil.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/WithStack/WithMessage/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cep21 please send a PR to this PR

@nmiyake
Copy link
Contributor

nmiyake commented Sep 24, 2016

Added PR to this PR to address the comment issue.

Anything else I can help with to advance this PR? I can fix merge conflicts with master if you let me know whether you prefer merging master in or rebasing the PR on master and fixing that way. If there are any TODO items that could be tackled I could take a look as well -- would really like to see this functionality soon!

davecheney and others added 6 commits September 29, 2016 11:14
Adds testFormatCompleteCompare as additional testing func.

The new function takes a string slice as "want", wherein
stacktraces and non-stacktrace messages are discerned by
strings.ContainsAny(want[i], "\n").

For example usage, see TestFormatWithStack & TestFormatWithMessage.
@davecheney davecheney merged commit 7433cb0 into master Sep 29, 2016
@davecheney davecheney deleted the destructure branch September 29, 2016 01:32
@dragonsinth
Copy link

I know this may be a bit silly, but WithMessagef would be nice addition as well. When converted "too many stack traces" code from Wrap -> WithMessage, converting Wrapf means an internal fmt.Sprintf().

@davecheney
Copy link
Member Author

#118 (comment)

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.

None yet

5 participants