Skip to content
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

Use go errors instead of github.com/pkg/errors #441

Merged
merged 1 commit into from
Jul 5, 2021

Conversation

sebastien-rosset
Copy link
Contributor

This PR is a proposal to remove the github.com/pkg/errors and replace it with the standard "errors" package.

  1. go 1.13 and above support the same or equivalent functionality compared to github.com/pkg/errors
  2. github.com/pkg/errors is now under maintenance

@puellanivis
Copy link
Collaborator

While most of pkg/errors has been superseded by errors itself, the standard errors package still does not attach any stack information to the error about where it occurred.

We need to decide if that stack information is useful or used anywhere first.

@sebastien-rosset
Copy link
Contributor Author

While most of pkg/errors has been superseded by errors itself, the standard errors package still does not attach any stack information to the error about where it occurred.

We need to decide if that stack information is useful or used anywhere first.

Understood. It's quite unusual for go programs to keep track of the error stack, though I understand coming from other languages this may be perceived as a gap.

@puellanivis
Copy link
Collaborator

It’s not all that unusual, because it gets dumped by any uncaught panic. There are good reasons for having a stack trace, and I have had to debug a lot of things in the last 7 years of working with in Go by using stack traces.

That said, I’m pretty sure we’re not using them properly here. I was just leaving in pkg/errors because it was there when I got here. 🤷‍♀️

@sebastien-rosset
Copy link
Contributor Author

Oh sure, yes, for panic. Sorry I meant for functions that return errors.

@puellanivis
Copy link
Collaborator

@drakkan I don’t really have any problems with us moving to standard lib errors, and removing the dependency on go.mod. I’m pretty sure there is nothing in the client that would make the additional stack traces useful.

If you’re good with this, go ahead and merge it, and let’s cut a new patch version with the bugfixes, otherwise, let’s close this, and cut a patch version with the bugfixes.

@drakkan drakkan merged commit eaa697c into pkg:master Jul 5, 2021
@someone1
Copy link

I was looking through the changelog before updating the package (great package btw 🎉 !) and thought I'd share this idea for this discussion:

I think, if all packages align properly, the error string will contain somewhat of a stack trace as to how it originated. Effective Go states:

When feasible, error strings should identify their origin, such as by having a prefix naming the operation or package that generated the error

So if a combination of packages returned errors such as:

if err != nil {
     return fmt.Errorf("package-name: error reading from file - %w", err)
}

we'd have a pseudo stack trace of all packages this error went through before being logged out as such.

Just my 2 cents!

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.

4 participants