-
Notifications
You must be signed in to change notification settings - Fork 375
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
Close()
should be Close() error
#82
Comments
I had an error return in there, but then removed it because it seemed to be a Potemkin village in most users. Everyone is all too eager to do defer somevariable.Close() ignoring the error, instead of writing something like defer func() { if e := somevariable.Close(); e != nil && err == nil { err = e } }() to propagate the error. So, would there be a practical benefit? Are there any interfaces which would benefit from implementing Arguably we should allow reporting the errors, which at least gives callers the option to handle failures correctly. The correctly-propagating-error handling could be actually done reasonably nicely with func CloseWithError(c io.Closer, pe *error) {
if e := c.Close(); e != nil && *pe == nil {
*pe = e
}
} and defer CloseWithError(somevariable, &err) This seems like such a commonly needed idiom that it should already exist somewhere in the standard library, but I can see this neither in |
even if people tends to do that no-err-check-defer-close it doesn't meant we should code to prevent that. Types and returns are documented and it's up to who implements this library to check errors if they think so (not questioning the rightness to not do that).
I believe so - it's a least future proof since many libraries and tools abstract writers and readers (and generally Closer(s)). |
ACK, let’s change that whenever someone has copious free time. |
yeah, wasn't mean as stuff to be done soon |
I'll do it |
FWIW, i think closewitherror is typically needed on Write/ReadClosers but not closers themselves -- it's typically used to propagate to io.Copy and similar. |
Do not get hung up on the |
Anything against having
{Image,ImageSource,ImageDestination}.Close()
implementio.Closer
https://golang.org/pkg/io/#Closer? (which basically is adding an error return). I think we would have this for #63 also.ref: https://github.com/containers/image/blob/master/types/types.go#L98
@mtrmac wdyt?
The text was updated successfully, but these errors were encountered: