-
Notifications
You must be signed in to change notification settings - Fork 50
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
codec/raw: implement the raw codec #150
Conversation
I also could not find a spec doc for it, otherwise I would have linked it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some nits to pick, but this is holistically pretty good.
It's small, it's simple, and it's already widely used as part of unixfs. So there's no reason it shouldn't be part of go-ipld-prime. The codec is tiny, but has three noteworthy parts: the Encode and Decode funcs, the cidlink multicodec registration, and the Bytes method shortcut. Each of these has its own dedicated regression test. I'm also using this commit to showcase the use of quicktest instead of go-wish. The result is extremely similar, but with less dot-import magic. For example, if I remove the Bytes shortcut in Decode: --- FAIL: TestDecodeBuffer (0.00s) codec_test.go:115: error: got non-nil error got: e"could not decode raw node: must not call Read" stack: /home/mvdan/src/ipld/codec/raw/codec_test.go:115 qt.Assert(t, err, qt.IsNil)
@warpfork ready, I think. |
// To disable the shortcut above, hide the Bytes method by wrapping the buffer | ||
// with an io.Reader: | ||
// | ||
// Decode([...], struct{io.Reader}{buf}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow, TIL. That's a neat idiom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one can accomplish it the explicit way, like I did in the test, but this shortcut is much shorter. It's not easy to realise it's possible unless you've seen it before, so... doc :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
(see commit message)