Encode, EncodeName: drop error from function signature

[flokli]

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]

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 ipld.NotFound break and maybe this would be just as bad since it's so low down in our stack

[masih]

I am curious why the existing implementation of Ecode does not perform validation; For example, is it OK to do multihash.Encode([]byte("fish"), multihash.SHA2_256) where the length of the given digest does not match the expected length of the given multihash code?

Assuming validation would be useful here, considering error is already in the function signature would it make sense to instead implement validation?

[ribasushi]

@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 https://dweb.link/ipfs/f0155121c8f71659370c5268d9a1dc962a46232540e8fca63462586d8efaa95aa https://dweb.link/ipfs/f015512188f71659370c5268d9a1dc962a46232540e8fca63462586d8

[masih]

Interesting; thanks @ribasushi

[rvagg]

but only if someone is willing to actually fix the fallout

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 EncodeName where it's not in Names rather than removing errors? No problems if you're not interested, I understand it's quite a departure from your original ask.

[flokli]

but only if someone is willing to actually fix the fallout

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

Well, this is a simple function signature change. On bumping the version, it's immediately shown by the compiler that the called function EncodeName only returns one parameter. It's a simple chore, I don't see how it can be that much of a pain.

@flokli I don't suppose you're interested in pivoting this PR to just return an error from EncodeName where it's not in Names rather than removing errors? No problems if you're not interested, I understand it's quite a departure from your original ask.

Everyone has been ignoring the err parameter so far anyways, I don't see how adding meaning now will actually get anyone to look at it :-/

[Stebalien]

@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.:

1. Package X updates go-multihash.
2. Package Y doesn't go-multihash.
3. Package Z depends on X and Y and can't update X until Y updates go-multihash.

Worse, many packages that depend on go-multihash aren't controlled by the same author.

Basically, either:

1. Everything needs to update in lock-step.
2. There needs to be a major version bump with a shim that aliases the new version's multihash type to the old versions multihash type (to avoid splitting the ecosystem across two versions).

[Stebalien]

Everyone has been ignoring the err parameter so far anyways, I don't see how adding meaning now will actually get anyone to look at it :-/

That's the user's problem, honestly. If users are ignoring errors, there's nothing we can do to help them.

[Stebalien]

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.

[flokli]

Everyone has been ignoring the err parameter so far anyways, I don't see how adding meaning now will actually get anyone to look at it :-/

That's the user's problem, honestly. If users are ignoring errors, there's nothing we can do to help them.

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 Encode, we document the error as "legacy", which further encourages people to ignore it:

So independent of the discussion on when/how we can change signatures, is this something that can be ignored or not?