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

Create new representation of Mulithash as a string. #82

Closed
wants to merge 4 commits into from

Conversation

kevina
Copy link
Contributor

@kevina kevina commented Aug 29, 2018

Unlike the the Cid type, the Multihash has a lot more historical baggage associated with it. It is many places assumed to be a []byte. A few functions in fact work with []byte directory instead of the Multihash type. So instead of changing the historical type I renamed it to MultihashBytes and created a new type that is based on a string. Uses of Multihash outside of the Cid can just use the MultihashByte type instead. There is very little code duplication and this way we don't break uses that assumed or required the Multihash to be []byte.

@Stebalien
Copy link
Member

I'd rather make a clean break or not do this. Having two is just going to be confusing and build up cruft.

@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

I'd rather make a clean break or not do this. Having two is just going to be confusing and build up cruft.

I attempted to do this at first but stopped due the the assumption that a Multihash is a []byte in many places. There may be use cases where having it as a string will create performance problems. Also the Multihash API could use a lot of work. I like to take this opportunity to provide a streamlined API. We can gradually remove parts of the Legacy API over time.

The multihash type itself is very simple so I don't see the problem with having an extra type. We already have a DecodedMultihash type and I think having a MultihashBytes type doesn't really add much complexity.

@Stebalien
Copy link
Member

Then let's just punt this to later. We don't currently use multihashes for blocks so this shouldn't yet be a performance issue. DecodedMultihash has a distinct and useful purpose, this is a stop-gap.

However, the CID stuff is a pressing performance issue so I'd like to do that now instead of tying these two together.

@Stebalien Stebalien added the status/deferred Conscious decision to pause or backlog label Aug 29, 2018
@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

Would you be okay with moving all the existing API into a separate package, call it oldmultihash? What will be kept in the new package is a new streamlined API that doesn't have any of the historical baggage associated with it. We can then gradually either just remove parts of the legacy API or if they end up being required move them into the new API.

@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

Then let's just punt this to later. We don't currently use multihashes for blocks so this shouldn't yet be a performance issue. DecodedMultihash has a distinct and useful purpose, this is a stop-gap.

If everything goes as plan that should be within a month or so.

@Stebalien
Copy link
Member

I've just done an initial conversion pass (complete migration) and it wasn't that painful. However, it isn't tied to the go-cid stuff in any way so I don't want to artificially tie them together.

@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

I've just done an initial conversion pass (complete migration) and it wasn't that painful. However, it isn't tied to the go-cid stuff in any way so I don't want to artificially tie them together.

I am confused as to what this is. Do you mean switching Multihash to a string? Or integrating this change?

@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

Also did you see my comment on:

Would you be okay with moving all the existing API into a separate package, call it oldmultihash?

Basically I would really like the opportunity to clean up and streamline the API during the conversion from changing the internal representation from []byte to string. I honestly think that keeping the old stuff around (that uses []byte) will help with this long term goal and also make the migration easier.

@Stebalien
Copy link
Member

I am confused as to what this is. Do you mean switching Multihash to a string? Or integrating this change?

I mean integrating the change: #83

Basically I would really like the opportunity to clean up and streamline the API during the conversion from changing the internal representation from []byte to string. I honestly think that keeping the old stuff around (that uses []byte) will help with this long term goal and also make the migration easier.

I'd rather not tie the two together unless the API changes are breaking (as far as I can tell, they mostly aren't).


Let's do the CID change for now, then this change. If we bundle too much together, we'll never finish anything (and it makes patches impossible to review).

@kevina
Copy link
Contributor Author

kevina commented Aug 29, 2018

Let's do the CID change for now, then this change. If we bundle too much together, we'll never finish anything (and it makes patches impossible to review).

Already done.

@kevina
Copy link
Contributor Author

kevina commented Aug 30, 2018

Okay. Closing this is favor of #84.

Please do not delete the branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/deferred Conscious decision to pause or backlog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants