davidhewitt / pythonize

MIT License
198 stars 28 forks source link

Fix deserializing `set` and `frozenset`. #39

Closed LilyFoote closed 1 month ago

LilyFoote commented 1 year ago

Adding these tests shows the following errors:

failures:

---- de::test::test_tuple_from_pyfrozenset stdout ----
thread 'de::test::test_tuple_from_pyfrozenset' panicked at 'called `Result::unwrap()` on an `Err` value: UnexpectedType("'frozenset' object cannot be converted to 'Sequence'")', src/de.rs:441:46
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- de::test::test_tuple_from_pyset stdout ----
thread 'de::test::test_tuple_from_pyset' panicked at 'called `Result::unwrap()` on an `Err` value: UnexpectedType("'set' object cannot be converted to 'Sequence'")', src/de.rs:441:46

failures:
    de::test::test_tuple_from_pyfrozenset
    de::test::test_tuple_from_pyset
codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 88.88% and project coverage change: +0.40 :tada:

Comparison is base (cfce252) 80.99% compared to head (858a3fb) 81.40%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #39 +/- ## ========================================== + Coverage 80.99% 81.40% +0.40% ========================================== Files 4 4 Lines 1021 1038 +17 ========================================== + Hits 827 845 +18 + Misses 194 193 -1 ``` | [Impacted Files](https://codecov.io/gh/davidhewitt/pythonize/pull/39?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt) | Coverage Δ | | |---|---|---| | [src/de.rs](https://codecov.io/gh/davidhewitt/pythonize/pull/39?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt#diff-c3JjL2RlLnJz) | `88.94% <88.88%> (+0.51%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Hewitt)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

davidhewitt commented 1 year ago

Thanks for this! This seems reasonable, though I'm also slightly cautious. What's the use case for this? I'm split down the middle on whether allowing set / frozenset as inputs to rust sequences makes sense. E.g. set/frozenset are unordered containers, but we are converting them to an ordered type?

To be fair, serde_json looks like it's doing the same e.g. HashSet becomes a JSON list.

LilyFoote commented 1 year ago

Thanks for this! This seems reasonable, though I'm also slightly cautious. What's the use case for this?

While I was trying to work out what the best way to handle my use case (see #38) I discovered that the set and frozenset code paths give an unexpected error. So this PR is avoiding that error in the way that fit with what the code seemed to be trying to do.

I'm split down the middle on whether allowing set / frozenset as inputs to rust sequences makes sense. E.g. set/frozenset are unordered containers, but we are converting them to an ordered type?

Yeah - the alternatives I can think of are:

To be fair, serde_json looks like it's doing the same e.g. HashSet becomes a JSON list.

Yeah, the serde docs (https://serde.rs/data-model.html#types) suggest this handling too, so it's consistent with rust stuff in general. But it does lose a bit of information, so I'm also not certain what's best.

LilyFoote commented 1 year ago

For my use case, an error immediately with #38 or something with newtype_struct (I'm really not sure exactly what this would look like) would probably be most useful.