Skip to content
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

merkledag: fix json marshalling of pbnode #3507

Merged
merged 2 commits into from
Dec 16, 2016
Merged

Conversation

whyrusleeping
Copy link
Member

previously ipfs dag get of a protobuf hash resulted in very unexpected output

License: MIT
Signed-off-by: Jeromy why@ipfs.io

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping whyrusleeping added this to the ipfs 0.4.5 milestone Dec 14, 2016
@whyrusleeping whyrusleeping requested review from a user and Kubuxu December 14, 2016 21:12
@whyrusleeping whyrusleeping added the status/in-progress In progress label Dec 14, 2016
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Wouldn't it be better to use JSON annotations?

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Nope, as those fields are private. What about unmarshalling it, is it needed?

@Kubuxu
Copy link
Member

Kubuxu commented Dec 14, 2016

I still can't figure the GH review stuff sometimes, sorry for the mess, it is now green, and I just wanted the red to go away.

@Kubuxu
Copy link
Member

Kubuxu commented Dec 14, 2016

I would be very happy with some unit tests for it 😄

A panic would occur when a link was created with a nil cid, this should
be allowable, just catch the potential problem and skip marshaling the
cid.

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping whyrusleeping merged commit 56bc19b into master Dec 16, 2016
@whyrusleeping whyrusleeping deleted the fix/pbnode-json branch December 16, 2016 00:41
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Dec 16, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants