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

Fix for #239: Union inside List or Dict is not deserialized as the correspond… #464

Merged

Conversation

pawelwilczewski
Copy link
Contributor

Fixed the issue described in #239.

I'm not sure this is the best way of fixing this as the comment suggests the type should have already been decoded but this is my best attempt.

Frankly, I haven't tested it properly (works for my use cases) but it seems to me the logic is valid.

I am not very familiar with the repo, so please let me know if I'm violating some of the rules set here and I'll try to adhere.

@pawelwilczewski
Copy link
Contributor Author

It would be good to add some test scenarios like the one in the mentioned issue.

Copy link
Collaborator

@george-zubrienko george-zubrienko left a comment

Choose a reason for hiding this comment

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

Hello @pawelwilczewski ! Thank you for the contribution - please add unit tests that back your claim the issue is fixed - and re-request a review ;)

@pawelwilczewski pawelwilczewski marked this pull request as draft August 7, 2023 23:08
@pawelwilczewski
Copy link
Contributor Author

Stole a little bit of test code from the issue - hopefully the author a-mckinley doesn't mind

@pawelwilczewski pawelwilczewski marked this pull request as ready for review August 8, 2023 00:39
@pawelwilczewski
Copy link
Contributor Author

@george-zubrienko done! Let me know if anything else needs changing!

@pawelwilczewski
Copy link
Contributor Author

I noticed it won't work with .schema.loads() because I never made changes to the mm.py. I'm afraid I won't have free time to get around to doing that in the nearest future...

@pawelwilczewski
Copy link
Contributor Author

I'd think a fix similar to what I did could be implemented in the else clause in line 87 in mm.py

@george-zubrienko
Copy link
Collaborator

I'd think a fix similar to what I did could be implemented in the else clause in line 87 in mm.py

I'll review what is here this week, then we'll open a new issue and somebody can pick it up ;)

@TristanSpeakEasy
Copy link
Contributor

I'd think a fix similar to what I did could be implemented in the else clause in line 87 in mm.py

I'll review what is here this week, then we'll open a new issue and somebody can pick it up ;)

would love to see this one go through as we have another PR that adds more support for undiscriminated unions we would like to go in after this pawelwilczewski#1

@pawelwilczewski
Copy link
Contributor Author

Not too sure why the poetry installation might have failed...

@george-zubrienko
Copy link
Collaborator

Merging #476 will fix, sorry for delays in reviewing, on my todo

@pawelwilczewski
Copy link
Contributor Author

No worries at all, and no pressure!

Copy link
Collaborator

@george-zubrienko george-zubrienko left a comment

Choose a reason for hiding this comment

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

LGTM, minor refactoring suggestion

dataclasses_json/core.py Outdated Show resolved Hide resolved
Co-authored-by: George Zubrienko <[email protected]>
@pawelwilczewski
Copy link
Contributor Author

Seems like there were some issues with the refactor. I'll roll back and you can automatically merge - unless you'd like to give it another go. Cheers!

@george-zubrienko
Copy link
Collaborator

I'll take a look why and suggest a new one. The one I added was from the phone so might have missed the if condition

@pawelwilczewski
Copy link
Contributor Author

Yeah I gave it an attempt but kinda just skimmed through it relying on github actions rather than properly testing

@george-zubrienko
Copy link
Collaborator

If you can fix would be great, if not I'll prepare a proper suggestion within next 2 days. I want this to be included in 0.6.0 :)

@pawelwilczewski
Copy link
Contributor Author

I believe this does it now

@george-zubrienko
Copy link
Collaborator

Awesome, thanks a lot for the contribution, will be released this weekend!

@george-zubrienko george-zubrienko merged commit 02c561f into lidatong:master Sep 1, 2023
9 checks passed
@pawelwilczewski
Copy link
Contributor Author

Happy to contribute. Thanks a lot for your overall and this PR's input!

renovate bot referenced this pull request in ixm-one/pytest-cmake-presets Dec 10, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [dataclasses-json](https://togithub.com/lidatong/dataclasses-json)
([changelog](https://togithub.com/lidatong/dataclasses-json/releases)) |
`^0.5.7` -> `^0.6.0` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/dataclasses-json/0.6.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/dataclasses-json/0.6.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/dataclasses-json/0.5.9/0.6.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/dataclasses-json/0.5.9/0.6.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the warning logs for
more information.

---

### Release Notes

<details>
<summary>lidatong/dataclasses-json (dataclasses-json)</summary>

###
[`v0.6.3`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.6.3)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.6.2...v0.6.3)

#### What's Changed

- Fixes catchall inheritance issue by
[@&#8203;jasonrock-a3](https://togithub.com/jasonrock-a3) in
[https://github.com/lidatong/dataclasses-json/pull/500](https://togithub.com/lidatong/dataclasses-json/pull/500)

#### New Contributors

- [@&#8203;jasonrock-a3](https://togithub.com/jasonrock-a3) made their
first contribution in
[https://github.com/lidatong/dataclasses-json/pull/500](https://togithub.com/lidatong/dataclasses-json/pull/500)

**Full Changelog**:
lidatong/dataclasses-json@v0.6.2...v0.6.3

###
[`v0.6.2`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.6.2)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.6.1...v0.6.2)

#### What's Changed

- fix: allow using CatchAll with postponed evaluation of annotations by
[@&#8203;2ynn](https://togithub.com/2ynn) in
[https://github.com/lidatong/dataclasses-json/pull/490](https://togithub.com/lidatong/dataclasses-json/pull/490)
- Fix PEP 0673 before 3.11 by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/487](https://togithub.com/lidatong/dataclasses-json/pull/487)
- fix mypy error when assigning to dataclass_json_config by
[@&#8203;MickaelBergem](https://togithub.com/MickaelBergem) in
[https://github.com/lidatong/dataclasses-json/pull/486](https://togithub.com/lidatong/dataclasses-json/pull/486)
- Fix an issue introduced with hetero tuple decode by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/493](https://togithub.com/lidatong/dataclasses-json/pull/493)

#### New Contributors

- [@&#8203;2ynn](https://togithub.com/2ynn) made their first
contribution in
[https://github.com/lidatong/dataclasses-json/pull/490](https://togithub.com/lidatong/dataclasses-json/pull/490)
- [@&#8203;MickaelBergem](https://togithub.com/MickaelBergem) made their
first contribution in
[https://github.com/lidatong/dataclasses-json/pull/486](https://togithub.com/lidatong/dataclasses-json/pull/486)

**Full Changelog**:
lidatong/dataclasses-json@v0.6.1...v0.6.2

###
[`v0.6.1`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.6.1)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.6.0...v0.6.1)

##### What's Changed

- Add links to make PyPI a better maintainer reference by
[@&#8203;pydanny](https://togithub.com/pydanny) in
[https://github.com/lidatong/dataclasses-json/pull/482](https://togithub.com/lidatong/dataclasses-json/pull/482)
- improve Union deserialization when "\__type" field specifier is not
present by [@&#8203;idbentley](https://togithub.com/idbentley) in
[https://github.com/lidatong/dataclasses-json/pull/478](https://togithub.com/lidatong/dataclasses-json/pull/478)

##### New Contributors

- [@&#8203;pydanny](https://togithub.com/pydanny) made their first
contribution in
[https://github.com/lidatong/dataclasses-json/pull/482](https://togithub.com/lidatong/dataclasses-json/pull/482)
- [@&#8203;idbentley](https://togithub.com/idbentley) made their first
contribution in
[https://github.com/lidatong/dataclasses-json/pull/478](https://togithub.com/lidatong/dataclasses-json/pull/478)

**Full Changelog**:
lidatong/dataclasses-json@v0.6.0...v0.6.1

###
[`v0.6.0`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.6.0)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.5.14...v0.6.0)

##### What's Changed

- Improve dataclass_json and \_process_class type annotations by
[@&#8203;ringohoffman](https://togithub.com/ringohoffman) in
[https://github.com/lidatong/dataclasses-json/pull/475](https://togithub.com/lidatong/dataclasses-json/pull/475)
- Update Poetry version used for 3.7 test suite and change
Requires-Python boundary by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/476](https://togithub.com/lidatong/dataclasses-json/pull/476)
- Fix for
[#&#8203;239](https://togithub.com/lidatong/dataclasses-json/issues/239):
Union inside List or Dict is not deserialized as the correspond… by
[@&#8203;pawelwilczewski](https://togithub.com/pawelwilczewski) in
[https://github.com/lidatong/dataclasses-json/pull/464](https://togithub.com/lidatong/dataclasses-json/pull/464)

Due to a behaviour change discovered in
[https://github.com/lidatong/dataclasses-json/issues/466](https://togithub.com/lidatong/dataclasses-json/issues/466)
and also as a matter of preparation for full deprecation of Py3.7, we
are bumping the minor version to 0.6.0. Most important change is that
since 0.5.9 builtins are coerced automatically without throwing an
exception. Please visit the issue for more info :)

##### New Contributors

- [@&#8203;ringohoffman](https://togithub.com/ringohoffman) made their
first contribution in
[https://github.com/lidatong/dataclasses-json/pull/475](https://togithub.com/lidatong/dataclasses-json/pull/475)
- [@&#8203;pawelwilczewski](https://togithub.com/pawelwilczewski) made
their first contribution in
[https://github.com/lidatong/dataclasses-json/pull/464](https://togithub.com/lidatong/dataclasses-json/pull/464)

**Full Changelog**:
lidatong/dataclasses-json@v0.5.15...v0.6.0

###
[`v0.5.14`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.5.14)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.5.13...v0.5.14)

#### What's Changed

- Temporarily disable code coverage publish for fork PRs by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/444](https://togithub.com/lidatong/dataclasses-json/pull/444)
- fix mashmallow fields.Tuple creation by
[@&#8203;healthmatrice](https://togithub.com/healthmatrice) in
[https://github.com/lidatong/dataclasses-json/pull/434](https://togithub.com/lidatong/dataclasses-json/pull/434)
- Allow the global config dictionary keys to also be Optional\[type] by
[@&#8203;sumnerevans](https://togithub.com/sumnerevans) in
[https://github.com/lidatong/dataclasses-json/pull/255](https://togithub.com/lidatong/dataclasses-json/pull/255)
- Fix global_config.mm_fields having no effect by
[@&#8203;sigmunau](https://togithub.com/sigmunau) in
[https://github.com/lidatong/dataclasses-json/pull/253](https://togithub.com/lidatong/dataclasses-json/pull/253)
- Fixes recursion bug related to enum flags by
[@&#8203;matt035343](https://togithub.com/matt035343) in
[https://github.com/lidatong/dataclasses-json/pull/447](https://togithub.com/lidatong/dataclasses-json/pull/447)
- Update Python version boundaries to include 3.12 by
[@&#8203;cdce8p](https://togithub.com/cdce8p) in
[https://github.com/lidatong/dataclasses-json/pull/449](https://togithub.com/lidatong/dataclasses-json/pull/449)

#### New Contributors

- [@&#8203;healthmatrice](https://togithub.com/healthmatrice) made their
first contribution in
[https://github.com/lidatong/dataclasses-json/pull/434](https://togithub.com/lidatong/dataclasses-json/pull/434)
- [@&#8203;sigmunau](https://togithub.com/sigmunau) made their first
contribution in
[https://github.com/lidatong/dataclasses-json/pull/253](https://togithub.com/lidatong/dataclasses-json/pull/253)
- [@&#8203;cdce8p](https://togithub.com/cdce8p) made their first
contribution in
[https://github.com/lidatong/dataclasses-json/pull/449](https://togithub.com/lidatong/dataclasses-json/pull/449)

**Full Changelog**:
lidatong/dataclasses-json@v0.5.13...v0.5.14

###
[`v0.5.13`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.5.13)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.5.12...v0.5.13)

#### What's Changed

- Fixes bug related to Tuples defined with ellipsis by
[@&#8203;matt035343](https://togithub.com/matt035343) in
[https://github.com/lidatong/dataclasses-json/pull/440](https://togithub.com/lidatong/dataclasses-json/pull/440)
- Revert type hint in annotation call by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/441](https://togithub.com/lidatong/dataclasses-json/pull/441)

**Full Changelog**:
lidatong/dataclasses-json@v0.5.12...v0.5.13

###
[`v0.5.12`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.5.12)

[Compare
Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.5.9...v0.5.12)

#### What's Changed

- Fix multiline scripts in CI by
[@&#8203;george-zubrienko](https://togithub.com/george-zubrienko) in
[https://github.com/lidatong/dataclasses-json/pull/439](https://togithub.com/lidatong/dataclasses-json/pull/439)

**Full Changelog**:
lidatong/dataclasses-json@v0.5.11...v0.5.12

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/ixm-one/pytest-cmake-presets).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi43OS4xIiwidXBkYXRlZEluVmVyIjoiMzcuODEuMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

---------

Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Izzy Muerte <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Izzy Muerte <[email protected]>
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.

Union inside List or Dict is not deserialized as the corresponding type
3 participants