container: clarify returned error #98
Conversation
pkg/container/reader.go
Outdated
| @@ -101,14 +101,12 @@ func (ctn Reader) GetToken(cid cid.Cid) (token.Token, error) { | |||
| } | |||
|
|
|||
| // GetDelegation is the same as GetToken but only return a delegation.Token, with the right type. | |||
| // If not found, delegation.ErrDelegationNotFound is returned. | |||
| func (ctn Reader) GetDelegation(cid cid.Cid) (*delegation.Token, error) { | |||
| tkn, err := ctn.GetToken(cid) | |||
| if errors.Is(err, ErrNotFound) { | |||
There was a problem hiding this comment.
Using errors.Is here implies the container can return other types of errors. If so, why was the next code block removed? If not, why not use err != nil here?
There was a problem hiding this comment.
That document the semantic ("this is the error I expected") ... but that can be a comment I suppose?
There was a problem hiding this comment.
That seems a bit fragile - if ctn.GetToken is later changed to return another type of error, tkn will be nil and you'll happily type that to a *delegation.Token on L110. Are we checking for a (*delgation.Token)(nil) everywhere downstream of this call?
There was a problem hiding this comment.
... and then you'd silently bubble up a new type of error, even though something should likely be done about it. I'm not sure there is a perfect solution here.
There was a problem hiding this comment.
I changed anyway.
MichaelMure
force-pushed
the
container-err
branch
from
9617005 to
6d7fd28
Compare
No description provided.