Closed rvagg closed 2 years ago
@rvagg IIRC the problem with just changing the values in the map (as done here) is that some of the map strings have changed in the process and there's no compiler-based indicators of the changes to the names used for dag-pb and dag-cbor, particularly dag-cbor where the previous name here "cbor" now means a different codec.
While the removal has some pain associated with it, at least it's explicit and users will notice it as compared to just changing the functionality and deprecating it.
If you need some help upgrading go-libp2p in go-bitswap I'd bet @Jorropo or @guseggert could help you out.
IIRC the problem with just changing the values in the map (as done here) is that some of the map strings have changed
Yep, that was the root problem, but maybe we should have deprecated rather than ripped them out. But also, the main problem is not Codecs
but CodecToStr
which goes the other way, where you usually want a user-printable form, which is reasonable, and is often not critical if it changes and all that code. In the libp2p instance it's simply to print a nicer error message: https://github.com/libp2p/go-libp2p-core/blob/243f8b9e3c8185705c55510a8c15c65abee739f6/peer/peer.go#L167-L171 - and now, we've replaced the exact behaviour but with go-multicodec: https://github.com/libp2p/go-libp2p/blob/8df365bf451b4df6998d4811c9f4139a862ac096/core/peer/peer.go#L160 printing a string form of a codec! So what we've gained is pushing people to a library with a more comprehensive, and generated list of codecs, great, but we've needlessly broken the ecosystem in the process when we could have just made a redirection from here to there with the same behaviour.
I could remove the Codecs
map, the value of that is a bit more dubious. But I still think Deprecated
is a better way of dealing with this when we're not doing proper (>0) semver.
Suggested version: v0.3.1
Comparing to: v0.3.0
(diff)
Changes in go.mod
file(s):
diff --git a/go.mod b/go.mod
index 6e3b580..8731e34 100644
--- a/go.mod
+++ b/go.mod
@@ -2,6 +2,7 @@ module github.com/ipfs/go-cid
require (
github.com/multiformats/go-multibase v0.0.3
+ github.com/multiformats/go-multicodec v0.5.0
github.com/multiformats/go-multihash v0.0.15
github.com/multiformats/go-varint v0.0.6
)
@@ -17,4 +18,4 @@ require (
golang.org/x/sys v0.0.0-20210309074719-68d13333faf2 // indirect
)
-go 1.17
+go 1.18
gorelease
says:
# github.com/ipfs/go-cid
## compatible changes
CodecToStr: added
Codecs: added
# summary
Suggested version: v0.4.0
gocompat
says:
(empty)
Uses go-multicodec as the source of truth this time.
So ... I think I want to assert that we made a mistake in https://github.com/ipfs/go-cid/pull/137 by removing it entirely. It continues to be a source of pain across our whole Go ecosystem of packages - primarily coming from the github.com/libp2p/go-libp2p-core usage of it in its
FromCid()
. Now, upgrading dependencies means following up a whole chain of (often unrelated) dependencies just to get libp2p upgraded. My latest instance of this is in trying to get some basic deps upgraded in go-merkledag, which shouldn't care about libp2p but does for some transitive dependencies in some test things. Ultimately that goes back to needing to upgrade go-bitswap's libp2p dependencies .. which I really don't want to have to go just to get go-merkledag upgraded.This time, we're deferring to go-multicodec for the mappings, and I'd like to get dependabot setup here too to make sure we keep that updated. There's also a deprecation notice which at least staticcheck should pick up.
I think, in lieu of proper semver-major semantics, this is playing like a good open source semver citizen, moreso than just yanking it out? Maybe next time for something like this we can bite the bullet and semver-major bump.