Encode, EncodeName: drop error from function signature#164
Encode, EncodeName: drop error from function signature#164flokli wants to merge 1 commit intomultiformats:masterfrom
Conversation
This makes it cleaner that this function doesn't return errors, shuts up linters complaining about ignoring errors etc. Considering go-multihash is v0.x.y, changing that signature should be fine.
rvagg
left a comment
rvagg
left a comment
There was a problem hiding this comment.
I think I'm in favour of this; I was just writing some code that used this and forgot that I didn't even need to write the error handling code.
We could bump minor version for this and just document it clearly.
@Stebalien you blocked the previous attempt to remove this @ #136 (comment) so you may want to weigh in here whether you still think it'd be annoying, even if we just did this as a single change with a semver-minor bump to go with it.
/cc @willscott @masih for more input if you have any
|
maybe I should be more cautious; I'm still having recurring pain from the go-cid break in 0.3 and the go-ipld-format |
|
I am curious why the existing implementation of Assuming validation would be useful here, considering |
|
@masih in CIDs/multihashes the length is indepenent from the hash-native length. These are all valid CIDs of the same underlying hash/content: https://dweb.link/ipfs/f015512208f71659370c5268d9a1dc962a46232540e8fca63462586d8efaa95aab492a208 |
|
Interesting; thanks @ribasushi |
Stebalien
left a comment
Stebalien
left a comment
There was a problem hiding this comment.
I would like this change, but only if someone is willing to actually fix the fallout. Which is going to be quite a bit of work for such a small change.
| // EncodeName is like Encode() but providing a string name | ||
| // instead of a numeric code. See Names for allowed values. | ||
| func EncodeName(buf []byte, name string) ([]byte, error) { | ||
| func EncodeName(buf []byte, name string) []byte { |
There was a problem hiding this comment.
We should be returning an error if the name isn't known. Otherwise, we just default to 0 which is definitely wrong.
yeah .. I'm having flashbacks to go-cid@0.3 pain and the way the libp2p breaking change straddle that and has taken a couple of years to get past; I don't think I want to deal with that I think we might have to pass on this. @flokli I don't suppose you're interested in pivoting this PR to just return an error from |
Well, this is a simple function signature change. On bumping the version, it's immediately shown by the compiler that the called function
Everyone has been ignoring the |
|
@flokli unfortunately, that's not the case due to dependency version rules and the fact that this repo contains a core type in interfaces, not some internal detail. Changing this is a massive pain. E.g.:
Worse, many packages that depend on go-multihash aren't controlled by the same author. Basically, either:
|
That's the user's problem, honestly. If users are ignoring errors, there's nothing we can do to help them. |
|
We'll eventually need to make breaking changes to this repo. E.g., to implement #29. That's a good reason to force the mass upgrade. Until then, I'm still of the opinion that this just isn't worth fixing. |
Well, that's what we do in the example code on the main README:
I mean, the comment is a bit confusing, because it doesn't elaborate which error can be ignored. But looking at the docstring of
So independent of the discussion on when/how we can change signatures, is this something that can be ignored or not? |
6 participants
This makes it cleaner that this function doesn't return errors, shuts up
linters complaining about ignoring errors etc.
Considering go-multihash is v0.x.y, changing that signature should be
fine.