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

Update to serde 1.0 #125

Merged
merged 1 commit into from
Aug 20, 2018
Merged

Conversation

jeandudey
Copy link
Contributor

@jeandudey jeandudey commented Aug 11, 2018

Status: Ready for review


@apoelstra
Copy link
Member

I think upgrading to serde 1.0 might be easier than trying to support both. I believe what needs to be done is:

  1. Update strason to 0.4 and secp256k1 to 0.10.0.
  2. Put strason, serde and the whole decimal module under a feature gate. (Later we can change decimal to be compatible with serde_jsons arbitrary_precision hack, drop the strason dependency, and make serde_json be a dev-dependency. But one step at a time.)
  3. Replace the serde_struct_impl macro in src/internal_macros.rs. You'll find the hardest thing to replicate is that it tries to recover from a missing field by calling MapVisitor::missing_field. The modern way[*] to do this is to require Default be implemented on optional fields, and use Default::default() in place of missing_field. But as far as I can tell we never have optional fields in rust-bitcoin structs, so it's fine to just drop this "feature".

[*] Ok, yes, the actual modern way is to use serde_derive, but this macro lets us avoid the compiler version bump.

@jeandudey jeandudey mentioned this pull request Aug 11, 2018
@jeandudey jeandudey force-pushed the 2018-08-11-serde-shim branch 3 times, most recently from 429095a to a1fde2a Compare August 12, 2018 15:05
@jeandudey jeandudey changed the title Coexisting implementations of serde-0.6 and serde-1.0 without breaking compatibility. Update to serde 1.0 Aug 12, 2018
@apoelstra apoelstra mentioned this pull request Aug 12, 2018
@jeandudey
Copy link
Contributor Author

@apoelstra strason 0.4 isn't compiling on 1.14.0

@jeandudey
Copy link
Contributor Author

jeandudey commented Aug 12, 2018

Also I've included your secp256k1 changes in this PR.

@apoelstra
Copy link
Member

Can you rebase now that we have secp 0.10 in?

@jeandudey
Copy link
Contributor Author

Done

src/lib.rs Outdated
extern crate serde;
extern crate strason;
#[cfg(feature = "serde")] extern crate serde;
#[cfg(any(feature = "serde-decimal", feature = "strason"))] extern crate strason;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this any isn't needed because serde-decimal implies strason?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK except for this. I think it can just be #[cfg(feature = "strason")]

The `serde_struct_impl!` macro has been modified to be compatible
with the serde 1.0 crate, we use this macro and not the `serde_derive`
crate because the latter doesn't support Rust 1.14.0 which is shipped
on Debian stable and we should remain compatible with it.

Two new features were added:

- "serde": enables serialization/deserialization for common types, it pulls
the serde 1.0 dependency.
- "serde-decimal": enables serialization/deserialization for `UDecimal`/`Decimal`,
this pulls the strason 0.4 depdendency and the serde 1.0 dependency.

Signed-off-by: Jean Pierre Dudey <jeandudey@hotmail.com>
@jeandudey
Copy link
Contributor Author

Done

@apoelstra apoelstra merged commit ec47b79 into rust-bitcoin:master Aug 20, 2018
@jeandudey jeandudey deleted the 2018-08-11-serde-shim branch August 20, 2018 18:24
Davidson-Souza pushed a commit to Davidson-Souza/rust-bitcoin that referenced this pull request Jul 12, 2023
Implement pre allocation context creation
yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024
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