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

Fixed #102, a message formatting bug. #114

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mrhwick
Copy link
Contributor

@mrhwick mrhwick commented May 4, 2017

Fixes #102

With these changes, when using a plain withMessage(), we'll print the additional message in front of the original error message, above the stack trace. This maintains the correct ordering when an error is being Wrap()'d, printing the message + second stack trace after the first message + stack trace.

@davecheney
Copy link
Member

Not LGTM sorry. Do we need a field on withMessage? Can't you do type assertion instead?

@mrhwick
Copy link
Contributor Author

mrhwick commented May 5, 2017

Essentially, there are two use cases we want to meet here.

  1. In the case where a withMessage is added to the chain independently of a withStack, we want to print them separately. In this case, we print the message prepended to the entire error format, and the additional stack appended to the error format.
  2. In the case where a Wrap() call adds a withMessage first, and then immediately adds a withStack, we want to keep them together because they are related. In this case, we would want to print the entire error format, then the withMessage and then the withStack. This will keep the message associated with its related stack.

If you use type assertion to accomplish this, you might do the type assertion in the withStack Format() function.

func (w *withStack) Format(s fmt.State, verb rune) {
	switch verb {
	case 'v':
		if s.Flag('+') {
			withMessageWrapper, ok := w.Cause().(*withMessage) // Type Assertion
			if ok { // Run this whenever we have a withMessage next in the chain
				fmt.Fprintf(s, "%+v\n", withMessageWrapper.Cause())
				io.WriteString(s, withMessageWrapper.msg)
				w.stack.Format(s, verb)
			} else { // Run this for anything else next in the chain
				fmt.Fprintf(s, "%+v", w.Cause())
				w.stack.Format(s, verb)
			}
			return
		}
		fallthrough
	case 's':
		io.WriteString(s, w.Error())
	case 'q':
		fmt.Fprintf(s, "%q", w.Error())
	}
}

This introduces an issue that from that scope there is no way to determine whether the withStack wraps a withMessage as a part of a Wrap() call. It could be a withMessage that should be kept separate, or it could be a withMessage that should be kept with its related withStack.

The scenario can easily occur where a pre-existing withMessage is placed in the chain before being passed to a WithStack() call. In these cases, the preexisting withMessage is indistinguishable from a withMessage added by calling Wrap(), and will be assumed to be related to the earlier withStack in the chain.

Maybe that is a viable solution? Is there a scenario where a WithMessage() followed next by WithStack() wouldn't be assumed to be related to one another?

@gregwebs
Copy link

Does it help to use causer instead of Cause for the type assertion to only go one level deep?

@hanzei
Copy link

hanzei commented Apr 17, 2019

What is the status of this PR? Does the comment above helped the discussion?

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.

WithMessage appends message after associated stack traces instead of before
4 participants