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

implement deserialize_reader #116

Merged
merged 3 commits into from
Jan 4, 2023

Conversation

Arshia001
Copy link
Contributor

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 the BorshDeserialize derive macro needs to be re-compiled, as the macro definition has changed.

@Arshia001
Copy link
Contributor Author

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 std::io::Write, so we wondered why std::io::Read wasn't supported in the deserializer. If there's a good reason, feel free to reject this change please :)

Copy link
Collaborator

@frol frol left a 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.

@Arshia001
Copy link
Contributor Author

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.

@Arshia001
Copy link
Contributor Author

@frol that's all the tests I can think of. If there's anything else, please let me know.

@frol
Copy link
Collaborator

frol commented Dec 22, 2022

@Arshia001 Please, address the clippy warnings

@Arshia001
Copy link
Contributor Author

@frol should be fixed now.

@frol frol merged commit 67448a3 into near:master Jan 4, 2023
@frol
Copy link
Collaborator

frol commented Jan 4, 2023

@Arshia001 Thank you for the contribution!

@Arshia001
Copy link
Contributor Author

@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.

@frol
Copy link
Collaborator

frol commented Jan 4, 2023

@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?

@Arshia001 Arshia001 mentioned this pull request Jan 8, 2023
@Arshia001
Copy link
Contributor Author

Sorry it took a while; I was sick these past few days.

This was referenced May 31, 2023
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.

None yet

2 participants