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

proposal: archive/zip: return clearer errors #53070

mbrackeantidot opened this issue May 25, 2022 · 3 comments

proposal: archive/zip: return clearer errors #53070

mbrackeantidot opened this issue May 25, 2022 · 3 comments


Copy link

Currently, the errors returned in archive/zip are often not very clear due to the following issues:

  • They lack documentation (e.g. ErrAlgorithm in reader.go on lines 25 and 190).
  • They are too generic to provide an actionable error message to the end user (e.g. ErrFormat in reader.go on lines 24 and 238).
  • They are not stored in a variable (e.g. the error in writer.go on line 512).
  • One return in particular seems to violate the documentation of io.Reader: on line 245 of reader.go we return 0, io.ErrUnexpectedEOF. But the documentation of io.Reader states "When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read." So we should return n instead of 0.

I would like to replace the current errors in archive/zip with actionable, clearly documented variables, like the ones we see in io/io.go. If you accept the general outline of the proposal, I can make a detailed design doc containing every error I would like to create. A design doc seems important given the number of public-facing changes that are needed.

@gopherbot gopherbot added this to the Proposal milestone May 25, 2022
Copy link

cc @dsnet

Copy link

I don't want to make you do extra work, but it's difficult to judge this proposal without a clear understanding of what would change. Can you at least give some specific examples? Thanks.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 25, 2022
Copy link

mbrackeantidot commented May 27, 2022

Don't worry about making me do extra work, I'm happy to provide clarification.
To make it as clear as possible what I want to do, I've made example commits in a fork of the repo, illustrating my proposal. Do please note that these are just examples, not commits I'd try to merge.

Here I replace a generic error with a specific, documented error that can be used to provide an actionable error message to the end user: b844a3b Rather than simply saying that the zip is invalid, it says what the problem is, making it easier for the user to figure out what went wrong.

Here I put an error in a documented variable: 982c85d The original error was hard to understand (I actually encountered it and had to go look in the source code to figure out what was going on.), but after this commit it can be found in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Status: Incoming

No branches or pull requests

4 participants