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

refactor: base32 encode CIDs by default #73

Closed
wants to merge 1 commit into from

Conversation

alanshaw
Copy link
Member

BREAKING CHANGE: toString and toBaseEncodedString will now use base32 encoding to encode version 1 CIDs by default.

BREAKING CHANGE: `toString` and `toBaseEncodedString` will now use base32 encoding to encode version 1 CIDs by default.

License: MIT
Signed-off-by: Alan Shaw <[email protected]>
@ghost ghost assigned alanshaw Jan 23, 2019
@ghost ghost added the status/in-progress In progress label Jan 23, 2019
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.

I'd like to get #72 merged first, which isn't a breaking change (as this change might need a lot of code changes).

@@ -110,7 +110,7 @@ describe('CID', () => {
expect(cid).to.have.property('version', 1)
expect(cid).to.have.property('multihash')

expect(cid.toBaseEncodedString()).to.be.eql(cidStr)
expect(cid.toBaseEncodedString('base58btc')).to.be.eql(cidStr)
Copy link
Member

Choose a reason for hiding this comment

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

Please change the expect to use no base parameter, else the test doesn't match its description and is the same test as the one above.

@alanshaw
Copy link
Member Author

Of course - there's absolutely no rush right now on getting this merged and released - I'll ping here when ready.

@vmx
Copy link
Member

vmx commented Jan 24, 2019

Good to know. Without any context I was under the impression it's one of those "last missing pieces" :)

@alanshaw
Copy link
Member Author

alanshaw commented Nov 5, 2019

No longer required - v1 CIDs already switched to using base32 encoding by default.

@alanshaw alanshaw closed this Nov 5, 2019
@alanshaw alanshaw deleted the refactor/cidv1base32-default branch November 5, 2019 11:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants