-
Notifications
You must be signed in to change notification settings - Fork 150
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
[BUG] Type coercion since 0.5.9 #466
Comments
This is not even type coercion, this is defo a regression because 1234 is not a boolean value. I'm looking into this. |
This is the culprit in
|
Now to sum up:
should be fine. Sames goes for floats etc. But for bool this goes side way because |
@george-zubrienko in v1 all these coercions should be a feature flag. It may still be the default behavior btw. |
Not sure minor bump is needed, since I would categorize it as a patch |
@USSX-Hares I agree that it should be a feature flag. @george-zubrienko please add to task list. I changed my mind, I recommend bumping to 0.6.0. |
Thanks everyone for the quick feedback! I still have some thoughts on this issue:
I do not use Python frameworks on a daily basis and I do not know any that would do that to strings and numerals. I think this is a bad idea, for three reasons:
Thanks again all! |
Python also has dynamic typing and is an interpreted language, the very existence of
This statement is the opposite of the first one. If we treat everything as recommendation, then we should have coercions as default - remember JSON is a text format and doesn't carry schema information like AVRO or PARQUET. In fact, it is a quite simple thing, how target types should be treated, but you seem to be more inclined towards strict compliance, which is one way to look at this, but not the only way ;)
I'd argue with this. As stated in the reasoning from the original author #375, since DCJ has
we should behave the way marshmellow users expect, and lack of type coercion was a bug, that turned out to be a feature for some. Thus, I agree that for some users - definitely, for others - long awaited fix that doesn't warrant a minor bump. However, even the boolean change I've opened a PR for can cause even more issues to be opened, where people either:
Thus, I will keep this issue opened and ref it to #442 where we will make both crowds happy by moving this behaviour to a feature flag, so everybody can have their own little DCJ doing the right thing. Until then, help welcome/stay tuned :) |
I don't want this to sidetrack into unrelated discussions and all, but I should point out that you are confusing dynamic/static with weak/strong typing. You should know that python is both dynamic AND strongly typed. Names that refer to objects are not bound to any type and may be reassigned freely. That's the dynamic part, and that's what That's the context of my intervention here. Like I said, you proceed as you see fit, but I do believe that treating python dataclasses as JSON/YAML-like objects (by default or not) is, though convenient, definitely not pythonic. 😅 |
That is exactly what is happening here. If you are unhappy with the response to the issue and you think some changes are in order, that is one thing and can be discussed, but so far I do not see any response to the reasons outlined in my response, including the In case you create an issue and put a bug label on it, we must investigate and decide on the best course of action, which includes you as the issue author, taking responsibility and helping us in this journey - for example push for minor version bump - do you think this is needed, or not? The
I never said the opposite :) Important part to note that we are dealing with text format serde in a language where you effectively have very little control of the actual type you are getting after deserialization, the magic "at runtime strictly typed" |
Those are fair points. My apologies. I do acknowledge there is a broader context here with DCJ, which I tend to forget. I mainly use |
Good, I tend to think we should have probably done this too, which @matt035343 agreed as well. But given that the damage has already been done, what do you think is the best course of action? Should we just bump the next release, will that be an acceptable solution? |
Bumping to a new version and then documenting seems to me the best thing to do in this case. In the release notes for 0.6.0, just specify that there's a fix for a regression introduced in 0.5.9, and then explain what is the intended behavior from this version onward. |
Alright, then this is what we'll do. I think we'll have some commits to release by end of this week, so expect 0.6.0 around Sunday :) |
Opened an issue for v1 - when we get there, would be nice if you have time to participate in the discussion :) Thanks a lot for bringing this up! |
Small update, moving this to next week as we might have another commit which can introduce some funky behaviours, so I'm pushing 0.6.0 to next release some time next week |
@jfsmith-at-coveo v0.6.0 released with a note on type coercion and a few other things. I'm closing this issue as resolved and promise this will be further addressed in v1 API when it comes out. |
[![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 [@​jasonrock-a3](https://togithub.com/jasonrock-a3) in [lidatong/dataclasses-json#500 #### New Contributors - [@​jasonrock-a3](https://togithub.com/jasonrock-a3) made their first contribution in [lidatong/dataclasses-json#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 [@​2ynn](https://togithub.com/2ynn) in [lidatong/dataclasses-json#490 - Fix PEP 0673 before 3.11 by [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#487 - fix mypy error when assigning to dataclass_json_config by [@​MickaelBergem](https://togithub.com/MickaelBergem) in [lidatong/dataclasses-json#486 - Fix an issue introduced with hetero tuple decode by [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#493 #### New Contributors - [@​2ynn](https://togithub.com/2ynn) made their first contribution in [lidatong/dataclasses-json#490 - [@​MickaelBergem](https://togithub.com/MickaelBergem) made their first contribution in [lidatong/dataclasses-json#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 [@​pydanny](https://togithub.com/pydanny) in [lidatong/dataclasses-json#482 - improve Union deserialization when "\__type" field specifier is not present by [@​idbentley](https://togithub.com/idbentley) in [lidatong/dataclasses-json#478 ##### New Contributors - [@​pydanny](https://togithub.com/pydanny) made their first contribution in [lidatong/dataclasses-json#482 - [@​idbentley](https://togithub.com/idbentley) made their first contribution in [lidatong/dataclasses-json#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 [@​ringohoffman](https://togithub.com/ringohoffman) in [lidatong/dataclasses-json#475 - Update Poetry version used for 3.7 test suite and change Requires-Python boundary by [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#476 - Fix for [#​239](https://togithub.com/lidatong/dataclasses-json/issues/239): Union inside List or Dict is not deserialized as the correspond… by [@​pawelwilczewski](https://togithub.com/pawelwilczewski) in [lidatong/dataclasses-json#464 Due to a behaviour change discovered in [lidatong/dataclasses-json#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 - [@​ringohoffman](https://togithub.com/ringohoffman) made their first contribution in [lidatong/dataclasses-json#475 - [@​pawelwilczewski](https://togithub.com/pawelwilczewski) made their first contribution in [lidatong/dataclasses-json#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 [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#444 - fix mashmallow fields.Tuple creation by [@​healthmatrice](https://togithub.com/healthmatrice) in [lidatong/dataclasses-json#434 - Allow the global config dictionary keys to also be Optional\[type] by [@​sumnerevans](https://togithub.com/sumnerevans) in [lidatong/dataclasses-json#255 - Fix global_config.mm_fields having no effect by [@​sigmunau](https://togithub.com/sigmunau) in [lidatong/dataclasses-json#253 - Fixes recursion bug related to enum flags by [@​matt035343](https://togithub.com/matt035343) in [lidatong/dataclasses-json#447 - Update Python version boundaries to include 3.12 by [@​cdce8p](https://togithub.com/cdce8p) in [lidatong/dataclasses-json#449 #### New Contributors - [@​healthmatrice](https://togithub.com/healthmatrice) made their first contribution in [lidatong/dataclasses-json#434 - [@​sigmunau](https://togithub.com/sigmunau) made their first contribution in [lidatong/dataclasses-json#253 - [@​cdce8p](https://togithub.com/cdce8p) made their first contribution in [lidatong/dataclasses-json#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 [@​matt035343](https://togithub.com/matt035343) in [lidatong/dataclasses-json#440 - Revert type hint in annotation call by [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#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 [@​george-zubrienko](https://togithub.com/george-zubrienko) in [lidatong/dataclasses-json#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 <63051+bruxisma@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Izzy Muerte <63051+bruxisma@users.noreply.github.com>
Description
I'll let you guys explain that this is intended and why, but starting with 0.5.9 there is type coercion going on when using the
from_dict()
method.A non-zero int becomes True on a bool field, for instance. An int becomes its stringified version on a string field.
This feels to me like a breaking change, but I don't see this anywhere in the release notes. Nor in the version number in any case.
Was this intended? If so, why?
Code snippet that reproduces the issue
Describe the results you expected
So this snippet raises with 0.5.8. Anything after and including 0.5.9 does not raise.
Python version you are using
3.11.4
Environment description
Tested with dataclasses-json 0.5.9, 0.5.12 and 0.5.14
The text was updated successfully, but these errors were encountered: