Skip to content
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

fix: bring back, but deprecate CodecToStr and Codecs #142

Merged
merged 2 commits into from
Sep 4, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 22, 2022

Uses go-multicodec as the source of truth this time.

So ... I think I want to assert that we made a mistake in #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.

@rvagg rvagg requested a review from lidel August 22, 2022 06:49
@aschmahmann
Copy link

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

@rvagg
Copy link
Member Author

rvagg commented Aug 23, 2022

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.

Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need this to update Kubo to go-libp2p v0.22.0 (I could also fix the dep but I'll see this later) LGTM.

@Jorropo Jorropo merged commit 69784e9 into master Sep 4, 2022
@Jorropo Jorropo deleted the rvagg/bring-back-CodecToStr branch September 4, 2022 11:03
@github-actions
Copy link

github-actions bot commented Sep 4, 2022

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants