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

feat: add util.cid options #15

Merged
merged 2 commits into from
Jun 25, 2018
Merged

feat: add util.cid options #15

merged 2 commits into from
Jun 25, 2018

Conversation

richardschneider
Copy link
Contributor

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.

Again thank you for adding the JSDocs, that's really useful.

it('.serialize and .deserialize', (done) => {
ipldGit.util.serialize(tagNode, (err, serialized) => {
expect(err).to.not.exist()
expect(Buffer.isBuffer(serialized)).to.equal(true)
Copy link
Member

Choose a reason for hiding this comment

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

Nit-pick: This can be written as expect(Buffer.isBuffer(serialized)).to.be.true(). There's no need to change it, it's just a note for future cases.

src/util.js Outdated
* @returns {void}
*/
exports.cid = (dagNode, options, callback) => {
if (options instanceof Function) {
Copy link
Member

Choose a reason for hiding this comment

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

Searching through the existing IPFS/IPLD code base it seems that the preferred way for checking if something is a function is: if (typeof options !== 'function') {. My guess is that it is related to code minifiers. Please change it.

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.

When you merge, please do a "squash merge" and use the commit message of the first commit. That gives a clean Git history.

@richardschneider richardschneider merged commit 5ed9c74 into master Jun 25, 2018
@richardschneider richardschneider removed the status/in-progress In progress label Jun 25, 2018
@richardschneider richardschneider deleted the cid-options branch June 25, 2018 07:48
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