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

feat!: split crates into multiple to isolate breaking changes #272

Merged
merged 39 commits into from
Apr 11, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Jan 31, 2023

Resolves #267.

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 was the one saying "why not keeping the serde-codec feature". Seeing the code, I don't it makes sense to have the serde and serde-codec feature. As this will be a breaking release, I think just going for serde is fine, it should be easy to change downstream.

codetable/Cargo.toml Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

I was the one saying "why not keeping the serde-codec feature". Seeing the code, I don't it makes sense to have the serde and serde-codec feature. As this will be a breaking release, I think just going for serde is fine, it should be easy to change downstream.

I am fine either way, I don't think it is too bad keeping it for now and mentioning it in the changelog that it is deprecated. Happy to remove it as well.

@thomaseizinger
Copy link
Contributor Author

As this will be a breaking release

My take on this would be that we should still try and make upgrading as easy as possible.

@thomaseizinger
Copy link
Contributor Author

Thanks for the prompt review @vmx! I addressed/responded all points, please take another look :)

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 think we are getting close!

Cargo.toml Outdated Show resolved Hide resolved
codetable/src/lib.rs Outdated Show resolved Hide resolved
codetable/tests/lib.rs Outdated Show resolved Hide resolved
codetable/src/lib.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
derive/src/hasher.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger requested a review from vmx March 23, 2023 14:10
codetable/src/lib.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger requested a review from vmx March 30, 2023 21:32
@thomaseizinger
Copy link
Contributor Author

@vmx Is something blocking this from being merged?

@thomaseizinger
Copy link
Contributor Author

Given the size of this change, I would suggest that we first cut an RC of everything and ask people if they are happy with this interface.

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.

Thanks a lot for all the work, nothing is blocking a merge.

Before doing a release, I'd like to get other breaking changes in, like removing the identity hash. Also some documentation (which I guess should go into the changelog) should be added to explain the upgrade path to the new structure.

I hope that downstream dependencies like libp2p, FVM, Forest etc would try things out to see if they can smoothly upgrade to this new version.

@vmx vmx merged commit 954e523 into multiformats:master Apr 11, 2023
@thomaseizinger
Copy link
Contributor Author

Before doing a release, I'd like to get other breaking changes in, like removing the identity hash. Also some documentation (which I guess should go into the changelog) should be added to explain the upgrade path to the new structure.

Definitely in favor of this. Happy to help, just ping me on some issues on what you would like to get done.

@thomaseizinger
Copy link
Contributor Author

I would still very much in favor of cutting an RC first and potentially an RC of multiaddr where we depend on the RC of multihash, just so downstream users can give us some feedback before we commit to the new interface.

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.

Tracking issue: Polish and stabilize the API
3 participants