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

fix: support for identity multihash #93

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Aug 8, 2019

Fixes #92 exception being throw by CID constructor when identity multihash is used

@Gozala Gozala changed the title feat: Add support for identity hashing fix: support for identity multihash Aug 8, 2019
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌

expect(cid0).to.have.property('codec', 'dag-pb')
expect(cid0).to.have.property('version', 0)
expect(cid0).to.have.property('multihash').that.eql(mh)
expect(cid0.toBaseEncodedString()).to.eql('161g3c')
Copy link
Member

Choose a reason for hiding this comment

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

FYI (this is not blocking a merge): you can also use toString() instead of toBaseEncodedString().

package.json Outdated Show resolved Hide resolved
@lidel
Copy link
Member

lidel commented Jun 15, 2020

@Gozala are you able to resolve conflicts?
@vmx would be good to merge those tests in.

@Gozala
Copy link
Contributor Author

Gozala commented Jun 15, 2020

@lidel thanks for reminding me of this pull (was staring at it puzzled not remembering doing it at all. Then noticed it's from last August 😱)

I'll resolve conflicts sometime today.

@Gozala Gozala requested a review from lidel June 15, 2020 16:13
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

lgtm – @vmx safe to merge (only adds tests)


Digression

@Gozala no worries, finding myself in similar situations sometimes, would be cool to see "stale PRs" in GitHub interface.

@andrew is there a way to get my own, swill open PRs that were hanging on GitHub for longer than two weeks etc? 🤔

@vmx
Copy link
Member

vmx commented Jun 16, 2020

It sadly needs another rebase.

@andrew
Copy link

andrew commented Jun 16, 2020

@lidel here you go: https://ecosystem-research.herokuapp.com/all?order=asc&sort=created_at&state=open&type=pull_requests&user=lidel

@lidel
Copy link
Member

lidel commented Jun 16, 2020

@vmx how do you mean? (CI is green, no conflicts)

@vmx
Copy link
Member

vmx commented Jun 16, 2020

@lidel Oh sorry. I had "Rebase and merge" selected (from a previous merge) and then it shows "This branch cannot be rebased due to conflicts")

image

But other ways of merging work.

@vmx vmx merged commit 51105b6 into multiformats:master Jun 16, 2020
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.

CID constructor throws when multihash uses "identity" algortihm
5 participants