search

lidatong / dataclasses-json

Easily serialize Data Classes to and from JSON
MIT License
1.34k stars 151 forks source link

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

Closed pawelwilczewski closed 10 months ago

pawelwilczewski commented 11 months ago

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 commented 11 months ago

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

pawelwilczewski commented 11 months ago

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

pawelwilczewski commented 11 months ago

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

pawelwilczewski commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 10 months ago

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 https://github.com/pawelwilczewski/dataclasses-json/pull/1

pawelwilczewski commented 10 months ago

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

george-zubrienko commented 10 months ago

Merging https://github.com/lidatong/dataclasses-json/pull/476 will fix, sorry for delays in reviewing, on my todo

pawelwilczewski commented 10 months ago

No worries at all, and no pressure!

pawelwilczewski commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

george-zubrienko commented 10 months ago

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 commented 10 months ago

I believe this does it now

george-zubrienko commented 10 months ago

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

pawelwilczewski commented 10 months ago

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