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

Remove deprecated packages #298

Merged
merged 3 commits into from
Aug 9, 2023
Merged

Conversation

harryzcy
Copy link
Contributor

@harryzcy harryzcy commented Aug 9, 2023

  • Remove github.com/pkg/errors, which is archived since 2021.
  • Remove ioutil, which is deprecated since Go 1.16

@coveralls
Copy link

coveralls commented Aug 9, 2023

Coverage Status

coverage: 85.97% (+0.02%) from 85.955% when pulling 3171cea on harryzcy:remove-deprecated into 151d2dc on jhillyerd:main.

@jhillyerd jhillyerd merged commit 3c35f97 into jhillyerd:main Aug 9, 2023
5 checks passed
@jhillyerd
Copy link
Owner

Thanks so much!

@harryzcy harryzcy deleted the remove-deprecated branch August 9, 2023 18:23
@dcormier
Copy link
Contributor

dcormier commented Aug 9, 2023

github.com/pkg/errors may be archived, but the stdlib errors package is, sadly, a poor replacement as it (still!) doesn't have a way of including stack information. Dave Cheney's even mentions this in his issue asking for new maintainers.

@jhillyerd
Copy link
Owner

Ah that's a bummer. I had looked in the docs to see if the stack was captured, but looking at my browser history, my search had ended up on pkg/errors, not stdlib errors. :/

@harryzcy
Copy link
Contributor Author

harryzcy commented Aug 9, 2023

I feel as a library, user are more concerned about if APIs succeed/failed in general, rather than in which part of the library, the error occurred. It's useful to know if email parsing succeed, if not, what's the reason that it failed.

So I feel exposing some public error types so that caller can check using stdlib functions is more important. Right now (either the pkg/errors or stdlib errors) basically returns a string message, and they are not super useful.

But if stack trace is really what you want, I can take another look to bring it back. But I feel pkg/errors is really too old, and maybe we can have our own solutions here.

@dcormier
Copy link
Contributor

I feel as a library, user are more concerned about if APIs succeed/failed in general, rather than in which part of the library, the error occurred. It's useful to know if email parsing succeed, if not, what's the reason that it failed.

I agree.

The stack is only visible when formatting errors with %+v. The usual %v (or even panic(err)) won't show the stack included with github.com/pkg/errors-wrapped errors. You can see a demonstration of this behavior here.

As someone who uses this package heavily and also has contributed to it, those stacks have been invaluable for helping track down problems and fixing them. It's why I asked for them in #94.

So I feel exposing some public error types so that caller can check using stdlib functions is more important.

That is still possible when when using the github.com/pkg/errors package to wrap errors. Errors using that package support the stdlib errors.Unwrap() function, which means the stdlib errors.Is() and errors.As() functions also work correctly. With error wrapping standardized since go1.13, errors.Is() and errors.As() are probably how you should be using to check whether an error is an expected value or type.

But if stack trace is really what you want, I can take another look to bring it back.

Even if one day enmime returns specific public error types/values for various errors, I feel having the stack is extremely valuable. Especially given the nested nature of MIME. This could possibly be overcome with some very thoughtful error design, but otherwise right now the errors returned by this package don't always give enough context to know where a problem might be.

But I feel pkg/errors is really too old, and maybe we can have our own solutions here.

It may be old, but it is extremely well used. As of right now, over 200K public imports.

@jhillyerd
Copy link
Owner

I agree w/ @dcormier -- debugging parsers is challenging, and real world email tends to be a bit of a torture test so having stack traces outside of a debugger can be helpful.

Probably best to revert the errors changes for the time being, but we can obviously keep the ioutil ones

jhillyerd added a commit that referenced this pull request Aug 14, 2023
jhillyerd added a commit that referenced this pull request Aug 15, 2023
@jhillyerd
Copy link
Owner

Ok, I've rolled back the errors changes, as I'd rather not release big changes to our error output twice. @harryzcy -- we can chat about alternate solutions in an new GH issue if you have ideas.

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