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

feat: upgrade to latest spec #17

Merged
merged 1 commit into from
May 22, 2016
Merged

feat: upgrade to latest spec #17

merged 1 commit into from
May 22, 2016

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented May 18, 2016

const src = {
data: 'hello world',
size: 11,
'@link': 'hello-world'
l1: {'&': 'hello-world'}
Copy link

Choose a reason for hiding this comment

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

sorry i think we went for /

may be good to make this a constant?

@dignifiedquire
Copy link
Member Author

I'm not a big fan of / but that's okay. It is a constant in the implementation but hard coded in tests to ensure correctness on change.

@@ -72,11 +70,15 @@ exports.unmarshal = (input, opts) => {
if (val instanceof cbor.Tagged) {
Copy link
Member

@daviddias daviddias May 19, 2016

Choose a reason for hiding this comment

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

Is it possible to use Duck Typing instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that will work very well, we want this exact thing, not something that looks like it

Copy link
Member

Choose a reason for hiding this comment

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

It is just because a tiny change in the version, a module that gets 'reused' and suddenly this verification doesn't work

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this can not happen here. We require cbor at the top and call cbor.decode and then check the decoded output against cbor.Tagged so this will always reference the same version of cbor.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, missed that, it is actually a very good point/guard. Can we add a comment so that the same question doesn't raise after?

@jbenet
Copy link

jbenet commented May 22, 2016

thanks for changing to /. this is close to merging, can we address the last few things? some people want to use the new ipld.

@daviddias
Copy link
Member

LGTM :)

@dignifiedquire dignifiedquire merged commit dd0a644 into master May 22, 2016
@dignifiedquire dignifiedquire deleted the upgrade branch May 22, 2016 08:52
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.

3 participants