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.

Sign up for GitHub

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

Jump to bottom

fix: comment from review #528

Merged
merged 2 commits into from
May 10, 2024
Merged

fix: comment from review #528

merged 2 commits into from
May 10, 2024

Commits on May 9, 2024

  1. fix: comment from review

    @PJCampi
    PJCampi committed May 9, 2024
    Configuration menu
    Copy the full SHA
    610b772 View commit details
    Browse the repository at this point in the history

  2. fix: fix failing tests in older versions of python. 

    In the failing tests, the dict value type is a Union of two dataclasses while the dict value instance is an instance of one of the two dataclasses.
If `is_dataclass` is asserted before `_is_supported_generic`, some tests fail because the value is a dataclass but `_decode_dataclass` is heavily relying on the type being a dataclass (f.ex. it calls `fields(cls)`) and Union isn't. If we unpack the union of dataclasses via _is_supported_generic first everything works fine. Hence my change.
Before my change, the code checked whether the **collection** was a dataclass (`is_dataclass(type_) or is_dataclass(xs)`) so the second assert was always false. I assumed it was just a bug. Our tests did not fail in that original code because the `is_dataclass(Union) or is_dataclass(dict)` predicate failed (the collection is not a dataclass) and then the code went on decoding the `Union` and that worked fine (which means that my incorrect code also passed all tests).
I can also roll back the refactoring to _decode_type and keep the code as it was (with the extra conditional branch for checking against the global configuration) if you think this is too risky.
It's a shame there is no test of the _is_dataclass(value) branch because I actually doubt that it ever worked (cause, as mentioned the class must also be a dataclass for things like `fields(cls)` to work)
    @PJCampi
    PJCampi committed May 9, 2024
    Configuration menu
    Copy the full SHA
    c209ba7 View commit details
    Browse the repository at this point in the history