-
Notifications
You must be signed in to change notification settings - Fork 153
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: support generic dataclass #525
fix: support generic dataclass #525
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matt035343 wdyt about this? Imo this allows you to squeeze lots of problems in as there are no bounds on T, which means it will always be a string/number/bool, what's the point then?
I am unsure what the current behavior is, crash and burn? In that case anything is better.
|
The current implementation will not decode the generic dataclass. The dataclass attribute will be assigned the json dict value instead. In my unittest, After my proposed change, whether the decoding works is down to whether T itself is supported by It is not a perfect solution at all but the library's coverage improves a little. I can add more tests to better highlight what works and what doesn't. |
Please do that - and request review again when ready :) |
Shouldn't we throw in case it is not supported? |
Maybe I should describe my change in more details because the name of the PR (and the ticket I opened) is a bit misleading. It's not about improving the decoding of type variables in a generic dataclass. That feature currently not supported by the library. The library sets the raw json node to a dataclass field of type T regarldess of its bound, constraints or annotation via
I am trying to align the behaviour of the library when a generic dataclass is used as type annotation to the behavior I just described. In the case where a generic dataclass (like Consider the following example:
On master we silently fail to decode the generic dataclass attribute
After my change we extract the concrete dataclass type from the generic wrapper. The type is now decoded as a dataclass with generic type
Throwing an exception instead of silently incorrectly setting a json node to an attribute of complex type would be a good idea but it's orthogonal to my change, and it would be a breaking change to the behaviour of the library. I added more test cases now. I did not write any test around type bounds or constraints cause that's not supported by the library anyways. |
I am convinced - @matt035343 any additional input or we release? |
I think it looks good |
To be released Monday :) fyi @PJCampi - and thanks for contributions and thorough explanations! |
[![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.6.4` -> `0.6.5` | [![age](https://developer.mend.io/api/mc/badges/age/pypi/dataclasses-json/0.6.5?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/dataclasses-json/0.6.5?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/dataclasses-json/0.6.4/0.6.5?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/dataclasses-json/0.6.4/0.6.5?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>lidatong/dataclasses-json (dataclasses-json)</summary> ### [`v0.6.5`](https://togithub.com/lidatong/dataclasses-json/releases/tag/v0.6.5) [Compare Source](https://togithub.com/lidatong/dataclasses-json/compare/v0.6.4...v0.6.5) #### What's Changed - fix: apply global config globally by [@​PJCampi](https://togithub.com/PJCampi) in [https://github.com/lidatong/dataclasses-json/pull/524](https://togithub.com/lidatong/dataclasses-json/pull/524) - fix: support generic dataclass by [@​PJCampi](https://togithub.com/PJCampi) in [https://github.com/lidatong/dataclasses-json/pull/525](https://togithub.com/lidatong/dataclasses-json/pull/525) #### New Contributors - [@​PJCampi](https://togithub.com/PJCampi) made their first contribution in [https://github.com/lidatong/dataclasses-json/pull/524](https://togithub.com/lidatong/dataclasses-json/pull/524) **Full Changelog**: lidatong/dataclasses-json@v0.6.4...v0.6.5 </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:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMjEuMiIsInVwZGF0ZWRJblZlciI6IjM3LjMyMS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZTpkZXBlbmRlbmNpZXMiXX0=--> Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
support generic dataclass