Skip to content

make the package compatible with go error package #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

meetme2meat
Copy link
Contributor

Fix: #32

Signed-off-by: Viren Negi <63040+meetme2meat@users.noreply.github.com>
@meetme2meat meetme2meat changed the title The the following PR make the package compatible with golang error package Make the package compatible with go error package May 11, 2021
@meetme2meat meetme2meat changed the title Make the package compatible with go error package make the package compatible with go error package May 11, 2021
@ConradIrwin
Copy link
Contributor

Interesting. I understand the problem (the interface value of error is not a nil interface because we're returning a typed nil from Wrap).

This is a backward incompatible change. That said, I think it would improve the library (and if I were writing it today, it'd probably be like you suggest).

On balance I'm going to merge this and add a section to the README that explains the breakage and how to work around it.

@ConradIrwin ConradIrwin merged commit 60174f7 into go-errors:master May 11, 2021
@meetme2meat
Copy link
Contributor Author

Great Thanks. Let me know when you shell out a new release I'm waiting on it.

@ConradIrwin
Copy link
Contributor

it's out already as 1.3.0

ConradIrwin added a commit that referenced this pull request May 14, 2021
…atabile-with-error-package"

This reverts commit 60174f7, reversing
changes made to 5c4c70f.
@ConradIrwin
Copy link
Contributor

@meetme2meat I ended up reverting this because it broke other people's workflows.

Unfortunately that means that the only path forward to fixing this is either a v2, or a separate API that does what you want.

@meetme2meat
Copy link
Contributor Author

yes, a v2 would be fine I can separate PR around it.

@waschik
Copy link
Contributor

waschik commented May 30, 2021

I am currently use code like this (I saved the if code to a template):

// g and h are functions in an external library not in my code.
func g() error {
	...
}

func h() error {
	...
}

func f() (err error) {
	errRaw := g()
	if errRaw != nil {
		err = errors.Wrap(errRaw, 0)
		return
	}

	errRaw = h()
	if errRaw != nil {
		err = errors.Wrap(errRaw, 0)
		return
	}
	return
}

This forwards only non nil errors. If error is nil I have no use for wrapping it. I have to check for nil anyway.

You could also use err directly for errRaw but for me it is more clear like this.

@gabrielf
Copy link
Contributor

gabrielf commented Mar 21, 2023

I think it makes sense to include the behavior in this PR in a version 2. In some cases you do have to check for nil anyway but in other cases it would simplify code if one could write:

func f() error {
    // …

    return errors.Wrap(g(), 0) // Always return != nil using latest go-errors
}

Especially now that Wrap and WrapPrefix includes the guards below it looks like the above code would work even though it won't:

func WrapPrefix(e interface{}, prefix string, skip int) error {
	if e == nil {
		return nil
	}

	// …
}

One thing that becomes slightly harder when New/Wrap/WrapPrefix returns error is to access the stack trace. I suggest that a new function errors.ErrorStack(err) is added to handle this.

Then you could write errors.ErrorStack(err) where you today write errors.Wrap(err, 0).ErrorStack(). The nil handling of this function can be discussed. It could be made to only work with non nil input:

// ErrorStack is a convenience function that returns an error's error message
// and callstack of a non nil error. If the provided error has no callstack
// one will be created to the call-site of this function.
func ErrorStack(err error) string {
	return Wrap(err, 1).(*Error).ErrorStack()
}

Or it could be made to handle nil by returning an empty string or <nil> or perhaps <nil> with a stack trace:

// ErrorStack is a convenience function that returns an error's error message
// and callstack. If the provided error has no callstack, one will be created to
// the call-site of this function. Calling this function with nil is most likely a bug
// but it will return a callstack to this function with the error message `<nil>`.
func ErrorStack(err error) string {
	if err == nil {
		return New(nil).(*Error).ErrorStack()
	}
	return Wrap(err, 1).(*Error).ErrorStack()
}

Because of the go gotcha where nil is not nil it may be safest to use the second implementation. One may use the go-errors library by the book but other code may still return a typed nil somewhere which will make your code end up treating a success like and error which may propagate all the way to a central error handler where the stack trace is extracted.

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

Successfully merging this pull request may close these issues.

nil error always fail at err != nil check
4 participants