search

multiformats / js-cid

CID implementation in JavaScript
MIT License
97 stars 39 forks source link

feat: preserve base when constructed from a string #77

Closed olizilla closed 5 years ago

olizilla commented 5 years ago

BREAKING CHANGE - previously base was not preserved and all CIDs would be normalised to base58btc when asked for their string representation.

The default will change to base32 in https://github.com/multiformats/js-cid/pull/73/files

The idea behind this change is that we shouldnt lose information when the user passes us a base encoded string, but keep it and use it as the default base so toString returns the same string they provided.

I'd like this as a fix for ipld explorer, which currently forces all CIDs into base58btc, seee: https://github.com/ipfs-shipyard/ipfs-webui/issues/999

This PR is the smallest change I can see to make it work. Do we want to also add base as a constructor parameter? If so, i guess it should go at the end as it's optional, but thats aesthetically displeasing as the base encoding goes at the front in string form.

fixes #76

License: MIT Signed-off-by: Oli Evans oli@tableflip.io

olizilla commented 5 years ago

I kept this PR small as this module could use a refactor, but I wanted to focus on the essence of the change first, but you are right, this code is harder to follow than it needs to be. I'll take another pass.

olizilla commented 5 years ago

@vmx I have a treat for you. I've clarified things, relied on spooky scopes less, and made it so each check in the constructor sets all the things on the new instance rather than maybe mutating the arguments, and return early from each one, so the state you have to keep in your head to figure out what its doing is shorter lived.

The tests pass locally for me, so I've no idea what travis is up to. Looking into it now.

olizilla commented 5 years ago

Oh commitlint! Subject must not end with a full stop!!! wtf. i. like. sentences.

Please can we kill commitlint for PRs... Its on the maintainer to squash and merge a PR branch with a commit message they are happy with. Getting a red x for a full stop in a commit message is demoralising, and I don't think it adds enough value to be worth that.

olizilla commented 5 years ago

Open questions:

vmx commented 5 years ago
  • Are two CIDs equal if they point to the same multihash but have a differenent default multibaseName for their string representation. I'm assuming yes.

Yes!

  • Should toV1 preserve an existing multibaseName if it's not the default? I've assumed no, as it's more useful it toV1 gives you a CID in the most common format.

I'm unsure, on one hand I'd say "no" as the most common case seems to be converting from V0 to V1. On the other hand I might expect a call to toV1() on a CID that is already V1 to be a no-op.

alanshaw commented 5 years ago

Please can we kill commitlint for PRs...

Yes please! https://github.com/ipfs/aegir/issues/345

olizilla commented 5 years ago

I got carried away and added the string caching that @alanshaw suggested. Sorry @vmx would you mind having another look.

Please feel free to squash and merge this to master as you see fit. I always hit the Squash and merge option in github and groom the commit message.

vmx commented 5 years ago

@olizilla I've edited the commit message a bit. I sadly can't a request a review from you, but feel free to thumb up this comment if you think it's ready to go (once the CI passed).

olizilla commented 5 years ago

@vmx I can't see what changed, but I think this PR is ready to go!

As per the commit-lint discussion, I tend to leave contributors PR branch commits as they provided them (feels nice, you see the progression, and they can refer back to it) and then use the Squash and merge feature to collapse it down to a single commit to master, where I get to groom the commit message for the change log.

vmx commented 5 years ago

@olizilla I just changed the commit message and nothing of the contents. What I started to do is having individual commits piled up and in the end to a squash locally as sometime i want to preserve some commits and force push that. This way I have CI running (and commit linting) and can be sure everything is fine.

vmx commented 5 years ago

@olizilla I just saw that the README isn't up to date about preserving the CID, would you mind updating it?

olizilla commented 5 years ago

Will do!