-
Notifications
You must be signed in to change notification settings - Fork 67
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
implement deserialize_reader #116
Conversation
I have no idea why this PR was created in the upstream repository, must have clicked the wrong button. Wanted to create it in our own. I'll take this as a sign and leave it here for review though. The serializer already supports |
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.
This is a breaking change. The old deserialize API is still available for consumers of the crate, but code that uses the BorshDeserialize derive macro needs to be re-compiled, as the macro definition has changed.
This does not sound like a breaking change to me if neither public interfaces nor serialization format is changed.
Overall, it seems to be a nice feature to have. Please, add tests. Unless anyone else raises their concern against merging it, I am happy to approve and merge this one.
Actually, you're right. It'd be a breaking change if you had pre-compiled code, but rust already compiles everything from scratch each time, so it's not. I'll get to work on the tests. |
@frol that's all the tests I can think of. If there's anything else, please let me know. |
@Arshia001 Please, address the clippy warnings |
@frol should be fixed now. |
@Arshia001 Thank you for the contribution! |
@frol you're welcome. Also, this is a breaking change after all; any manually implemented deserializers will need be reimplemented over the new trait. I don't know how likely users are to have manually implemented deserializers, but this change will definitely break them. |
@Arshia001 Ah, indeed, I missed that bit. I will tick the minor version and indicate this breakage in the release notes. May I ask you to add a record about this feature and the breaking change to the CHANGELOG.md? |
Sorry it took a while; I was sick these past few days. |
Add support for deserialization of data within an
std::io::Read
.This is a breaking change. The old
deserialize
API is still available for consumers of the crate, but code that uses theBorshDeserialize
derive macro needs to be re-compiled, as the macro definition has changed.