-
Notifications
You must be signed in to change notification settings - Fork 31
Feature complete #4
Conversation
fc414a7 to
fb964fd
Compare
|
@dignifiedquire @victorbjelkholm could you review? Thank you :) |
test/index.spec.js
Outdated
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.
Could you please move this to multihashing-async, we don't want slow tests :)
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.
Definitely something happening next week, however, it would be impossible for me to focus on the IPLD work at hand, if adding to the npm link mad mountain that I had to do, I also had to maintain branches of branches npm linking all over the place :)
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.
Fair point, starting Monday I will fully focus on getting that merged so we can upgrade these branches and ship ALL THE THINGS.
src/codecs.js
Outdated
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.
Would be nice if we could just pull this directly off of js-multicodec
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.
💡 Or even a multicodec-table module that gets published directly from the spec repo, this way, all the other multi* can require it and use always the latest.
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.
👍
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.
do you plan on doing this?
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.
Once we have multiformats/multicodec#16 finished, otherwise, instead of having codes floating in two places, we would have it floating in three
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.
Added a note here: multiformats/multicodec#20
package.json
Outdated
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.
TODO: remove before merge
src/codecs.js
Outdated
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.
do you plan on doing this?
src/index.js
Outdated
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.
Please throw some kind of error on the non working cases. This is a new format and could easily lead to implicit errors if we fail silently
src/index.js
Outdated
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.
lets add at least some basic validations
|
Thank you for the CR @dignifiedquire, applied, now validation happens and it will throw if something is incorrect. |
update multibase and multicodec add tests add documentation
d9bd1e2 to
211970b
Compare
Support for both v0 and v1 CID.
Ready for review