pkg / errors

Simple error handling primitives
https://godoc.org/github.com/pkg/errors
BSD 2-Clause "Simplified" License
8.2k stars 697 forks source link

Proposal: export `causer` interface #176

Closed JensRantil closed 5 years ago

JensRantil commented 5 years ago

Background: I have a function that will return two categories of errors, validation errors and call it...non-validation errors. Depending on error category, I will return an HTTP 4XX or 5XX error. I'd prefer to not have to manually check every type of validation error that the function could return, so I've introduced:

type ValidationError struct{ cause error }

func (w *ValidationError) Error() string { return "token validation error: " + w.cause.Error() }
func (w *ValidationError) Cause() error  { return w.cause }

func IsValidationError(err error) bool {
    // This is the interface that pkg/errors uses internally (and has
    // publically documented).
    type causer interface{
            error
            Cause() error
        }
    for {
        if _, ok := err.(ValidationError); ok {
            return true
        }
        if c, ok := err.(causer); ok {
            err = c.Cause()
        } else {
            return false
        }
    }
}

(code untested, apologize for small bugs but I think you get the gist) When I call my function I'll wrap the validation errors in this type to signal that it's a validation error. I attended a Go meetup in Stockholm this past week where a couple of other companies also expressed they were doing something similar to signal a specific error.

Proposal: Make the causer interface public/exported. Two reasons:

An alternative route: One way would be to create a function Untangle(err error) ([]error) which will return a list of all wrapped errors. This would avoid exporting the interface, but at the same be able to inspect the error hierarchy.

Additionals: Do you have any alternative approach to this specific problem? I was surprised that I couldn't find any proposal for this, so let me know if this has already been discussed previously.

davecheney commented 5 years ago

Thank you for raising this issue. I’m afraid I’m not going to make this change.

The causer interface is unexported to discourage people from importing this package just to get access to the interface. That is not necessary because you can use any interface, or define your own, that offers a Cause() error message.

On 2 Dec 2018, at 08:56, Jens Rantil notifications@github.com wrote:

Background: I have a function that will return two categories of errors, validation errors and call it...non-validation errors. Depending on error category, I will return an HTTP 4XX or 5XX error. I'd prefer to not have to manually check every type of validation error that the function could return, so I've introduced:

type ValidationError struct{ cause error }

func (w ValidationError) Error() string { return "token validation error: " + w.cause.Error() } func (w ValidationError) Cause() error { return w.cause }

func IsValidationError(err error) bool { // This is the interface that pkg/errors uses internally (and has // publically documented). type causer interface{ error Cause() error } for { if _, ok := err.(ValidationError); ok { return true } if c, ok := err.(causer); ok { err = c } else { return false } } } (code untested, apologize for small bugs but I think you get the gist) When I call my function I'll wrap the validation errors in this type to signal that it's a validation error. I attended a Go meetup in Stockholm this past week where a couple of other companies also expressed they were doing something similar to signal a specific error.

Proposal: Make the causer interface public/exported. Two reasons:

For similar cases as above to not having to define the causer interface ourselves. The causer interface is sort of "already out there" since it's documented in the pkg/errors package documentation. An alternative route: One way would be to create a function Untangle(err error) ([]error) which will return a list of all wrapped errors. This would avoid exporting the interface, but at the same be able to inspect the error hierarchy.

Additionals: Do you have any alternative approach to this specific problem? I was surprised that I couldn't find any proposal for this, so let me know if this has already been discussed previously.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

JensRantil commented 5 years ago

✅Thanks for considering it!

nhooyr commented 5 years ago

That is not necessary because you can use any interface, or define your own, that offers a Cause() error message.

Not sure what you mean here. I think this should be reconsidered to codify the expectations of this package for custom errors. Furthermore, it would enable tests to ensure types implement the correct interface.

davecheney commented 5 years ago

@nhooyr the name of the interface is not important, only the method set. Anyone can define their own interface with a Cause() error method or declare one inline.

nhooyr commented 5 years ago

Why should they define their own? Why not just use a shared standard interface set by this package?

davecheney commented 5 years ago

To reduce software coupling. The caller should define the interface, not the implementer.

On 5 Mar 2019, at 09:32, Anmol Sethi notifications@github.com wrote:

Why should they define their own? Why not just use a shared standard interface set by this package?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

nhooyr commented 5 years ago

I don't see how this reduces software coupling, all it does is make the coupling dynamic.

The caller should define the interface, not the implementer.

If the caller defines the interface, how is the implementer supposed to use it without knowing what it is in advance?

davecheney commented 5 years ago

My concern is if I publish an interface type then people will import the errors package just to use it. This creates a strong source level coupling which is unnecessary.

On 5 Mar 2019, at 11:38, Anmol Sethi notifications@github.com wrote:

I don't see how this reduces software coupling, all it does is make the coupling dynamic.

The caller should define the interface, not the implementer.

If the caller defines the interface, how is the implementer supposed to use it without knowing what it is in advance?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

puellanivis commented 5 years ago

If the caller defines the interface, how is the implementer supposed to use it without knowing what it is in advance?

This question is difficult to answer, because the naive answer to it would simply be an explanation of how interfaces work in Go. 😕