Improve Union type support #87

ydylla · 2021-02-10T22:40:23Z

This tries to add support for Unions with complex types like UUID or IPv4Address.

The basic idea is to use the code generation to add function for each Union the dataclass uses.
This union function then tries to serialize or deserialize the field according to the order of types defined in the Union.
The first one that works is used.

Using the code generation for this has the advantage that the user can influence the order of checks and that only types are checked that are defined in the Union.
Which should be faster, then always checking against all types that pyserde supports.

Currently, not all tests work. I have not found a good solution for nested Unions or container types within Unions, e.g. Union[List[int],List[str]].
We probably need some form of recursion.

Some notable other changes:

Implemented is_set in compat.py
The rendered dict & iter functions now define a variable serde_scope which I needed to pass it to the union functions.
The render functions now render NoneType as None. I think this broke the test_union_optional test, but I still have to look into it.

@yukinarit If you have time, I would appreciate a review and your opinion about the general idea?
Also I noticed you increased the version to 0.2.3 so only at patch level.
Do you use semantic versioning? Then it should be 0.3.0 in my opinion because the new type support is a feature 😄.

Closes #60

yukinarit

@ydylla
Thank you for the contribution! Sorry for the delay in getting back to you because it was a holiday in Japan 😉

I left review comments. Please check 👍

serde/core.py

serde/de.py

yukinarit · 2021-02-12T13:43:11Z

Do you use semantic versioning? Then it should be 0.3.0 in my opinion because the new type support is a feature 😄.

Closes #60

Yes I treat pyserde versioning as 0.MAJOR.MINOR as discussed in this issue, but am not 100% sure this is right. How do you treat versioning before 1.0.0? 🤔

ydylla · 2021-02-12T18:54:30Z

Yes I treat pyserde versioning as 0.MAJOR.MINOR as discussed in this issue, but am not 100% sure this is right. How do you treat versioning before 1.0.0? 🤔

In my projects I do not treat 0.x.x specially. I start with 0.1.0 and then increase minor 0.2.0 for features and patch 0.2.1 for fixes.
So once I need to assign a version and have other people using it I follow semantic versioning.
If I have to break something I would increase the major version despite semver allowing breaking changes in 0.x.x

* passing union args per serde scope to rendered union function * spliting is_instance into multiple functions & rendering the correct one will be faster

yukinarit · 2021-02-14T12:52:17Z

If I have to break something I would increase the major version despite semver allowing breaking changes in 0.x.x

Oh! I didn't know sember allows breaking change before 1.0.0 😲
I still don't want to update the major version because pyserde is not stable enough.

Anyway, I will change it from 0.2.3 to 0.3.0 maybe after a few more MRs are merged. 👍
Let me know If you need a new version pyserde in PyPI

* is_instance takes some shortcuts to increase performance, like only checking the 1st element of lists * typecheck was only used in tests

To be more performant we just try to deserialize instead of checking types, this means the order of types in unions is important. More complex types should be tried first, otherwise simple types will prematurely match.

* use a typed dataclass as container for serde internals * removed more_types.py & the custom serializer code. Unkown type exception has now own function.

class inheritance caused overwriting of scope values

thanks tomlfile example, otherwise I had not noticed it

codecov · 2021-02-26T22:02:48Z

Codecov Report

Merging #87 (e02e1ee) into master (6c4a832) will increase coverage by 1.03%.
The diff coverage is 92.37%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   87.15%   88.18%   +1.03%     
==========================================
  Files          11       11              
  Lines         903      948      +45     
  Branches      187      203      +16     
==========================================
+ Hits          787      836      +49     
+ Misses         79       75       -4     
  Partials       37       37

Impacted Files	Coverage Δ
serde/inspect.py	`51.85% <25.00%> (ø)`
serde/core.py	`85.61% <82.89%> (+3.35%)`	⬆️
serde/compat.py	`95.23% <96.15%> (+1.90%)`	⬆️
serde/__init__.py	`100.00% <100.00%> (ø)`
serde/de.py	`91.76% <100.00%> (+0.13%)`	⬆️
serde/json.py	`100.00% <100.00%> (ø)`
serde/msgpack.py	`100.00% <100.00%> (ø)`
serde/se.py	`94.60% <100.00%> (+0.22%)`	⬆️
serde/toml.py	`100.00% <100.00%> (ø)`
serde/yaml.py	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c4a832...e02e1ee. Read the comment docs.

ydylla · 2021-02-26T22:25:22Z

Hi @yukinarit,
this is now ready to be reviewed, so if you have time please take a look.

I am not sure if you like 431c914 but I thought it would be nice (and cleaner) to use the serde scope like an actual scope, which contains all serde internals. This way we do not need several __foo__ names to hide the internal functions. In the generated functions the scope is now always available as serde_scope (for jinja and within the functions).

If you do not like it, it is no problem to get the union support working without it. So do not feel obliged to merge this.

yukinarit

Hi @ydylla
I don't mind replacing scope object with dataclass. I left a few comments in order to understand the intent of the code. Please answer to the question when you have time.

Thanks!

serde/compat.py

yukinarit · 2021-03-04T12:51:38Z

LGTM

ydylla · 2021-03-04T17:47:38Z

🥳 Thanks for the merge.

yukinarit · 2021-03-04T22:20:58Z

No problem! Thanks for the contribution! 🙂

yukinarit reviewed Feb 12, 2021

View reviewed changes

serde/core.py Outdated Show resolved Hide resolved

serde/de.py Show resolved Hide resolved

ydylla mentioned this pull request Feb 13, 2021

Add support for Set #90

Merged

ydylla added 4 commits February 13, 2021 23:09

test: add more union tests (#60)

5fdb76a

feat: add union support for complex types

434edf6

fix: test_union_optional

e86b345

wip: use own is_instance for se

af84ab3

* passing union args per serde scope to rendered union function * spliting is_instance into multiple functions & rendering the correct one will be faster

ydylla added 11 commits February 22, 2021 21:20

chore: remove redundant checks for bare types

e48c770

chore: replace typecheck with is_instance

a86d9b4

* is_instance takes some shortcuts to increase performance, like only checking the 1st element of lists * typecheck was only used in tests

test: fix deserializing test by reordering types in union

36c7095

To be more performant we just try to deserialize instead of checking types, this means the order of types in unions is important. More complex types should be tried first, otherwise simple types will prematurely match.

Merge remote-tracking branch 'origin/master' into unions

4a3c384

chore: fix set support

1210fe1

refactor: use __serde__ as scope for everything

431c914

* use a typed dataclass as container for serde internals * removed more_types.py & the custom serializer code. Unkown type exception has now own function.

fix: scope should not be shared between classes

889ada1

class inheritance caused overwriting of scope values

style: format code

8deaa3a

test: fix bug in iter func test

5e8d2e5

fix: use iter_unions to recursively collect all unions of dataclass

577aeb9

thanks tomlfile example, otherwise I had not noticed it

test: omit reason because it depends on python version

3a973f9

ydylla changed the title ~~WIP: Improve Union type support~~ Improve Union type support Feb 26, 2021

yukinarit reviewed Mar 1, 2021

View reviewed changes

serde/compat.py Show resolved Hide resolved

refactor: use a single dict for all generated functions

e02e1ee

yukinarit merged commit 52d4522 into yukinarit:master Mar 4, 2021

ydylla deleted the unions branch May 8, 2021 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Union type support #87

Improve Union type support #87

ydylla commented Feb 10, 2021 •

edited

yukinarit left a comment

yukinarit commented Feb 12, 2021

ydylla commented Feb 12, 2021

yukinarit commented Feb 14, 2021

codecov bot commented Feb 26, 2021 •

edited

ydylla commented Feb 26, 2021

yukinarit left a comment

yukinarit commented Mar 4, 2021

ydylla commented Mar 4, 2021

yukinarit commented Mar 4, 2021

Improve Union type support #87

Improve Union type support #87

Conversation

ydylla commented Feb 10, 2021 • edited

yukinarit left a comment

Choose a reason for hiding this comment

yukinarit commented Feb 12, 2021

ydylla commented Feb 12, 2021

yukinarit commented Feb 14, 2021

codecov bot commented Feb 26, 2021 • edited

Codecov Report

ydylla commented Feb 26, 2021

yukinarit left a comment

Choose a reason for hiding this comment

yukinarit commented Mar 4, 2021

ydylla commented Mar 4, 2021

yukinarit commented Mar 4, 2021

ydylla commented Feb 10, 2021 •

edited

codecov bot commented Feb 26, 2021 •

edited