-
Notifications
You must be signed in to change notification settings - Fork 59
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
Conversation
There was a problem hiding this 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.
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. |
My take on this would be that we should still try and make upgrading as easy as possible. |
We don't want this in our dependency tree.
Not common enough to variant a type alias.
2b426e8
to
aab78df
Compare
Thanks for the prompt review @vmx! I addressed/responded all points, please take another look :) |
There was a problem hiding this 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!
@vmx Is something blocking this from being merged? |
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. |
There was a problem hiding this 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.
Definitely in favor of this. Happy to help, just ping me on some issues on what you would like to get done. |
I would still very much in favor of cutting an RC first and potentially an RC of |
Resolves #267.