-
Notifications
You must be signed in to change notification settings - Fork 225
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
Support deserializing from borrowed or owned bytes, too #668
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.
Could you also bump the patch minor version and update the changelog? Thanks!
Could we also add support for deserializing from a serde Roughly the encoding of this structure:
|
I've added support for sequences of bytes, too. That seemed natural and reasonable to support (and indeed it is what @ordian re version bumping and changelog: The changelog has an unreleased seciton already and the version isn't bumped from that last change. Should I either:
|
I was suggesting 1, but if you're more comfortable with 2, that works too :) |
I'm happy with 1; happy to just publish the crate too once it's merged but wanted to check that there wasn't any particular release approach/policy to follow if so :) Edit: Ah I found the CONTRIBUTING.md! One thing to note is that a publish of this crate (impl-serde) would also require a publish and update of a bunch of other crates that depend on older versions of this to actually put it into use, though. Perhaps I'll just publish this one first and can worry about updating the other crates in this repo that depend on it later; wdyt? |
Yup, I can take care of the rest. It seems to be another semver-pocalypse which would necessitate a breaking bump for almost all crates. |
I was wondering whether we could have gotten away with a patch bump instead? I've also been mulling over this and I think I'd like to add a followup PR to accomodate another deserialization woe (basically; the type info for types like H256 indicates that they are some newtype surrounding an array of bytes, and so it would be nice to try and support that, too, in the deserialization, rather than the current stuff which is all good but pretends that we're going straight to an array of bytes.) |
I've bumped patch versions on MSRV bump previously and people were not happy about that: #620. The MSRV is not caused by your PR, but an unreleased change. I understand you want to get this out quickly and easily, but we gotta do it the right way.
It seems the crate hasn't been published, so feel free to submit additional PRs and change the date of the release in the changelog. |
Support deserializing from bytes, too (see #669).
Closes #669