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

chore(ipld): switch to using top-level ipld-prime codec helpers #383

Merged
merged 3 commits into from
May 31, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 27, 2022

Ref: ipld/go-ipld-prime#428

since the bindnode usage here gets a few recommendations as an example usage, this switches to the top-level encode/decode helpers to encourage that as best-practice (one big benefit is that we can avoid the whole Representation() thing on encode). Also less code, so yay.

@rvagg rvagg requested a review from hannahhoward May 27, 2022 09:14
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Approved. Can we go a step further and simply remove these functions?

@rvagg
Copy link
Member Author

rvagg commented May 31, 2022

Yeah, I'll have a go. They're mainly helpful now for abstracting away the use of dagcbor, which is pretty trivial I suppose, they're all single-line functions.

@@ -8,6 +8,7 @@ import (
blocks "github.com/ipfs/go-block-format"
cid "github.com/ipfs/go-cid"
"github.com/ipld/go-ipld-prime"
_ "github.com/ipld/go-ipld-prime/codec/dagcbor"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only awkward bit of removing ipldutil.go entirely - it preloaded dagcbor and raw codecs; but there are some places that use testchain that don't touch the codecs directly but they're encountered by the loader because testchain uses a dagcbor cidlink.LinkPrototype.
But I think it's not unreasonable to add this here since it's also referencing cid.DagCBOR in this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds fine. oy. I wonder if we should just preload them in the root package at some point

Copy link
Member Author

Choose a reason for hiding this comment

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

my initial take was to put them both there - but when you use graphsync as a whole (not just as a unit test) you're going to get dagcbor no matter what because it's in the messaging layer. raw is going to come if you need it, but that's up to the user IMO, maybe they want to add dagjson, dagjose, git, and more, but that's up to them so I'm fine with localising this I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool.

@rvagg rvagg requested a review from hannahhoward May 31, 2022 03:16
@rvagg
Copy link
Member Author

rvagg commented May 31, 2022

Removed ipldutil.go entirely, pretty clean

@hannahhoward hannahhoward merged commit fa5a9f2 into main May 31, 2022
@rvagg rvagg deleted the rvagg/top-level-gip-decode-encode branch May 31, 2022 06:12
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.

2 participants