implement Serialize, Deserialize for Py<T> #1366
Conversation
daniil-konovalenko
force-pushed
the
serde
branch
from
1719ba3
to
e7225a4
Compare
|
@davidhewitt hi, could you please take a look at this? The PR is by no means ready for the real review, but I would really appreciate your feedback on the general implementation idea. The thing that bothers me the most is error handling — should we just convert pyo3 errors to String and create custom serde errors from it, or maybe we should attach more context to them somehow? |
|
Thanks!
I don't see any clear problem... what do you have in your mind as a blocker?
I think both |
|
Thank you for the quick response!
I was anxious about error handling (I'm new to Rust), and the tests could be better, but the implementation itself looks ready for review. Also I haven't written the docs yet, but this doesn't block the review of course |
guide/src/features.md
Outdated
| @@ -28,6 +28,27 @@ These features are an extension of the `abi3` feature to specify the exact minim | |||
|
|
|||
| See the [building and distribution](building_and_distribution.md#minimum-python-version-for-abi3) section for further detail. | |||
|
|
|||
| ### `serde` | |||
There was a problem hiding this comment.
This needs to go in the "Advanced Features" section (which should maybe be renamed to "Other Features") as this feature could be useful in the embedding case too.
There was a problem hiding this comment.
fixed, thanks!
Cargo.toml
Outdated
|
|
||
| [dev-dependencies] | ||
| assert_approx_eq = "1.1.0" | ||
| trybuild = "1.0.23" | ||
| rustversion = "1.0" | ||
| proptest = { version = "0.10.1", default-features = false, features = ["std"] } | ||
| # features needed to run the PyO3 test suite | ||
| pyo3 = { path = ".", default-features = false, features = ["macros", "auto-initialize"] } | ||
| pyo3 = { path = ".", default-features = false, features = ["macros", "auto-initialize", "serde"] } |
There was a problem hiding this comment.
Strictly speaking, we don't need this feature to run the tests - they straight up don't work without the other features enabled.
I'd rather just see the serde feature added to the list of features tested in ci.yml.
There was a problem hiding this comment.
Thanks for the tip! Added serde to ci.yml
There was a problem hiding this comment.
Thanks, LGTM!
@kngwyu unless you think otherwise, I'm going to merge this after we release 0.13.1, to allow some time to get feedback on this on master before this makes a release.
|
I force-pushed a squash and rebase; will merge assuming CI is happy. Thanks again for the contribution! |
David Hewitt wrote in #1358
I thought this too and hacked together a
"serde"feature that includes implementations of Serialize and Deserialize forPy<T>TODO: