add rkyv support #520
add rkyv support #520
Conversation
There was a problem hiding this comment.
Thanks for the contribution - seems simple enough! There are a few additional changes to get this ready for merge:
- Can you add a test for this? Possibly follow the same style as
borsh. You'd also need to add a new test into the Makefile and "turn it on" for CI here: https://github.com/paupino/rust-decimal/blob/master/Makefile.toml#L176 - Add a section to the readme for this feature. It doesn't have to be much! https://github.com/paupino/rust-decimal/blob/master/README.md#borsh
- Some minor tweaks as described in the
Cargo.tomlfile in regards todefault-features- it helps us keep build times low.
Cargo.toml
Outdated
| @@ -35,6 +35,7 @@ rocket = { default-features = false, optional = true, version = "0.5.0-rc.1" } | |||
| serde = { default-features = false, optional = true, version = "1.0" } | |||
| serde_json = { default-features = false, optional = true, version = "1.0" } | |||
| tokio-postgres = { default-features = false, optional = true, version = "0.7" } | |||
| rkyv = {version = "0.7", optional = true} | |||
There was a problem hiding this comment.
nit: can we do a couple of things here:
- this list is alphabetical - can we put it before
rocket? The parameters are also alphabeticalized. - Can we set
default-features = falseand include any required features explicitly?
dovahcrow
force-pushed
the
master
branch
2 times, most recently
from
9eec22f
to
dde70fb
Compare
|
@paupino Thank you for the detailed suggestions! I've revised the PR accordingly. This project is so well maintained! I appreciate all the time & effort you put on this project. |
|
rkyv also allows one to use a safe api with a feature flag (https://github.com/rkyv/rkyv#example). Do you want to enable that by default? FYI safe API allows one to deserialize types from bytes with some additional checks (e.g. byte alignment), with some performance sacrifice. |
There was a problem hiding this comment.
One minor change required with the feature flags (std isn't quite right).
rkyv also allows one to use a safe api with a feature flag (https://github.com/rkyv/rkyv#example). Do you want to enable that by default?
Your call! I'm happy with how it is right now but if you think it's a useful feature then feel free to add it.
Cargo.toml
Outdated
| @@ -68,7 +70,7 @@ serde-str = ["serde-with-str"] | |||
| serde-with-arbitrary-precision = ["serde", "serde_json/arbitrary_precision", "serde_json/std"] | |||
| serde-with-float = ["serde"] | |||
| serde-with-str = ["serde"] | |||
| std = [] | |||
| std = ["rkyv/std"] | |||
There was a problem hiding this comment.
This isn't quite right, but I see what you're trying to do. Perhaps for now, we can move the rkyv/std feature under rkyv also. It just means that the feature requires the std feature. I'll make a code suggestion for above.
There was a problem hiding this comment.
| std = ["rkyv/std"] | |
| std = [] |
There was a problem hiding this comment.
I think std = ["rkyv/std"] means if the std feature of rust-decimal is turned on, also turn on the std feature of rkyv?
There was a problem hiding this comment.
Unfortunately, it includes the dependency by default too. See this section: https://doc.rust-lang.org/cargo/reference/features.html#dependency-features
I see that "rkyv?/std" is actually what we want however it requires a minimum rust version of 1.60. Unfortunately, it'll have to wait until the min version requirements catch up: https://github.com/paupino/rust-decimal#minimum-rust-compiler-version
There was a problem hiding this comment.
I see. right "rkyv?/std" is the correct one.
Cargo.toml
Outdated
| @@ -59,6 +60,7 @@ default = ["serde", "std"] | |||
| legacy-ops = [] | |||
| maths = [] | |||
| maths-nopanic = ["maths"] | |||
| rkyv = ["rkyv/size_32"] | |||
There was a problem hiding this comment.
| rkyv = ["rkyv/size_32"] | |
| rkyv = ["rkyv/size_32", "rkyv/std", "std"] |
There was a problem hiding this comment.
rkyv can be used without std. I added rkyv/std in the std feature of rust_decimal
Oh I just saw your last comment.
dovahcrow
force-pushed
the
master
branch
4 times, most recently
from
a55d9a1
to
61c06e6
Compare
|
@paupino Fixed the std issue and also added safe API support via the |
This PR add optional rkyv support for Decimal.