Skip to content
This repository has been archived by the owner on Aug 11, 2021. It is now read-only.

fix: pass serialized blob to util.cid #67

Merged
merged 2 commits into from
Jun 26, 2018
Merged

fix: pass serialized blob to util.cid #67

merged 2 commits into from
Jun 26, 2018

Conversation

richardschneider
Copy link
Contributor

BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32

BREAKING CHANGE: the first argument is now the serialized output NOT the dag node.
See ipld/interface-ipld-format#32
@richardschneider richardschneider requested a review from vmx June 25, 2018 09:43
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Just minor things.

src/util.js Outdated
@@ -116,14 +117,15 @@ exports.deserialize = (data, callback) => {
/**
* Get the CID of the DAG-Node.
*
* @param {Object} dagNode - Internal representation
* @param {Buffer} blob - Output from serialize()
Copy link
Member

Choose a reason for hiding this comment

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

It's true that it is the output of of serialize(). What about making it clearer with also saying that it is CBOR encoded binary data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Serialized binary data"?

Copy link
Member

Choose a reason for hiding this comment

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

"Serialized binary data"?

It's binary data always the serialized one? What about "Encoded CBOR"? I don't have a strong opinion on that, so I can also live with keeping it as it currently is.

Copy link
Member

Choose a reason for hiding this comment

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

"Serialized binary data"?

It's binary data always the serialized one? What about "Encoded CBOR"? I don't have a strong opinion on that, so I can also live with keeping it as it currently is.

Edit: I should be clearer. When someone is reading just the JSDocs. Would you know what needs to be passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted a description that could be used for all IPLD formats.

So CBOR is out.

Encoded begs the question "What is the encoding". By using Serialized we are indirectly naming the function to encode.

serialize returns a buffer, which is binary data.

Copy link
Member

Choose a reason for hiding this comment

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

If you want something generic, then I think the original Output of serialize() was best. Though I don't know if a generic comment makes sense. Shouldn't the API docs be about that specific implementation? Most people will use this code through js-ipld, there the comment will be generic. But in case you use one format directly, you want to know about the specifics.

@@ -49,7 +49,7 @@ describe('dag-cbor interop tests', () => {
// put it back to bytes
node[0]['/'] = bs58.decode(arrayLinkJSON[0]['/'])

dagCBOR.util.cid(node, (err, cid) => {
dagCBOR.util.cid(arrayLinkCBOR, (err, cid) => {
Copy link
Member

Choose a reason for hiding this comment

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

The deserialize() call isn't needed anymore as cid() uses the serialized node. As deserialize() (hopefully :) is tested somewhere else, the deserializing step should be removed from this test.

It's the same for the tests below, I won't mention it again for all the individual tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt unconformable deleting the code from a test. But you are right.

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

The only thing left is the JSDoc description of blob. As I said, I don't have a strong opinion, hence I approve this PR, feel free to merge it.

@richardschneider richardschneider merged commit 1ec7744 into master Jun 26, 2018
@@ -116,14 +117,15 @@ exports.deserialize = (data, callback) => {
/**
* Get the CID of the DAG-Node.
*
* @param {Object} dagNode - Internal representation
* @param {Buffer} blob - Serialized binary data
Copy link
Member

Choose a reason for hiding this comment

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

The Github UI isn't good with changes. So I repost it (slightly different though :) I would say something like Encoded CBOR blob or Serialized as CBOR.

@@ -116,14 +117,15 @@ exports.deserialize = (data, callback) => {
/**
* Get the CID of the DAG-Node.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it should say Get the CID of the CBOR node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about Get the CID of the serialised CBOR node?

@richardschneider richardschneider mentioned this pull request Jun 26, 2018
richardschneider added a commit that referenced this pull request Jun 27, 2018
richardschneider added a commit that referenced this pull request Jun 27, 2018
* Revert "Revert "docs: improve (#68)" (#69)"

This reverts commit 7ba6589.

* Revert "docs: improve (#68)"

This reverts commit 9dc56ad.

* Revert "fix: pass serialized blob to util.cid (#67)"

This reverts commit 1ec7744.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants