More types & some refactoring #80
Conversation
* msgpack: classes don't need a special _type attribute anymore. The ext_dict contains all information we need.
* se.py now internally uses to_obj similar to de.py
* add **opt to Nested class to work with reuse_instances
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
==========================================
- Coverage 88.35% 85.74% -2.61%
==========================================
Files 10 11 +1
Lines 773 863 +90
Branches 162 178 +16
==========================================
+ Hits 683 740 +57
- Misses 61 83 +22
- Partials 29 40 +11
Continue to review full report at Codecov.
|
|
I totally forgot that this also adds support for date & datetime from datetime 😃 |
python 3.6 has no fromisoformat functions so we have to ship our own (copied from python 3.8)
|
Hi @ydylla, Thank you for the huge contribution! Let me check little by little 👍 |
|
Okay I will change it. I also guessed this was the reason 😄 |
serde/msgpack.py
Outdated
| ext_type_code = None | ||
| if ext_dict is not None: | ||
| obj_type = type(obj) | ||
| ext_type_code = next((code for code, ext_type in ext_dict.items() if obj_type is ext_type), None) |
There was a problem hiding this comment.
ext_type_code = ext_dict.get(obj_type, None)
seems simpler
There was a problem hiding this comment.
I know these kind of one liners are bad practice.
But I wanted both functions to accept the same ext_dict: Dict[int, Type] (like in the test).
And for to_msgpack we have to get the key (integer type code) of the dict by searching with its value (the type).
A simple ext_dict.get( only works for the other direction. See from_msgpack.
There was a problem hiding this comment.
On second thoughts I think it would be more efficient if the from_msgpack accepted a reversed dict. That way, the app would do the one time reversal and lookups would be efficient in both directions.
There was a problem hiding this comment.
I think you meant to_msgpack because from_msgpack already uses a simple lookup.
Yes two dicts would be more efficient/faster. Another way could be to add the ext_type_code directly to the to_msgpack function as argument. Maybe the app already knows which type code to use.
In the original code the type code was saved as _type attribute. In my opinion the knowledge about the name of this special attribute should not be part of pyserde.
But with the ext_type_code as argument one could use it like this:
d = DerivedA(i=1, s="a", j=10)
to_msgpack(d, ext_type_code=d._type)So basically the app has to implement the lookup itself.
There was a problem hiding this comment.
@yukinarit Thanks for the review.
I changed to_msgpack to use a Dict[Type, int] but I am not really happy with it
I guess it will confuse at least some users and also forces the application to save two dicts with basically the same information. But for know it is good enough, maybe someone else has a better idea in future.
Also keep in mind this is now a breaking change.
|
@ydylla |
|
I'm neutral. No big deal either way.
…On Tue, Feb 2, 2021 at 5:22 AM yukinarit ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In serde/de.py
<#80 (comment)>:
> @@ -379,7 +379,7 @@ def opt(self, arg: DeField) -> str:
exists = f'{arg.data} is not None'
else:
exists = f'{arg.datavar}.get("{arg.name}") is not None'
- return f'{self.render(value)} if {exists} else None'
+ return f'({self.render(value)}) if {exists} else None'
Is this parenthesis necessary? 🤔
------------------------------
In serde/__init__.py
<#80 (comment)>:
> @@ -16,7 +16,7 @@
'is_deserializable',
'from_dict',
'from_tuple',
- 'asdict',
- 'astuple',
+ 'to_dict',
+ 'to_tuple',
Thanks for the explanation! I am curious of other people's opinion 🙂
@adsharma <https://github.com/adsharma> @jfuechsl
<https://github.com/jfuechsl> @andreymal <https://github.com/andreymal>
@alexmisk <https://github.com/alexmisk> @pranavvp10
<https://github.com/pranavvp10>
There is a proposal to renaming asdict/astuple to to_dict/to_tuple
What do you guys think? 🤔
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFA2AYCPCFX36YFM37HV3TS474ANANCNFSM4W3Q3O7A>
.
|
Done, I found it to verbose to add all types of |
|
I'd say it's a question of preference. |
|
@adsharma @jfuechsl
Yeah, from_dict/to_dict naming sounds more consistent with other formats but renaming at this time is confusing for the exiting users (although pyserde user base is still small). @ydylla |
|
I readded |
|
The code that uses these APIs is here:
https://github.com/adsharma/raft/tree/master/raft/messages
I like the reversed dicts to keep the symmetry. The only reason why _type
exists is for serialization/pyserde.
We can discuss a different name if that's inconvenient.
…On Wed, Feb 3, 2021 at 12:22 PM ydylla ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In serde/msgpack.py
<#80 (comment)>:
> """
If `ext_dict` option is specified, `obj` is encoded as a `msgpack.ExtType`
"""
- return se.serialize(obj, **opts)
+ ext_type_code = None
+ if ext_dict is not None:
+ obj_type = type(obj)
+ ext_type_code = next((code for code, ext_type in ext_dict.items() if obj_type is ext_type), None)
I think you meant to_msgpack because from_msgpack already uses a simple
lookup.
Yes two dicts would be more efficient/faster. Another way could be to add
the ext_type_code directly to the to_msgpack function as argument. Maybe
the app already knows which type code to use.
In the original code the type code was saved as _type attribute. In my
opinion the knowledge about the name of this special attribute should not
be part of pyserde.
But with the ext_type_code as argument one could use it like this:
d = DerivedA(i=1, s="a", j=10)to_msgpack(d, ext_type_code=d._type)
So basically the app has to implement the lookup itself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFA2A355QN645PU56WEAPLS5GV63ANCNFSM4W3Q3O7A>
.
|
|
LGTM 👍 |
|
I will merge once this thread is resolved 👍 |
A single ext_type_code argument would probably be even better, so that the application code can implement the lookup however it likes
|
@adsharma Is everything ok? Could you check if your reviews were all addressed? |
|
Everything looks good and tests pass. Thanks for making the changes!
I need to push a small commit for the reversed dict once this is merged.
…On Mon, Feb 8, 2021 at 3:59 AM yukinarit ***@***.***> wrote:
@adsharma <https://github.com/adsharma> Is everything ok? Could you check
if your reviews were all addressed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFA2AY3AJAGJ772E4L6OJDS57G2ZANCNFSM4W3Q3O7A>
.
|
|
Nice. Thanks @yukinarit and @adsharma. |
This pull request adds support for more types and also utilizes the code generation capabilities for them.
The following types where added:
I needed these types because I wanted to use pyserde in combination with asyncpg.
asyncpg returns dict like objects which can be converted to dataclasses by pyserde's
from_dictfunction.Since asyncpg also supports the above mentioned types, the dicts already contain instances of them.
That is the reason why I also added a new argument
reuse_instancesforfrom_dict&from_tuple.When this is set to
Truethese functions check if the field already contains an instances of the target type and reuse it when possible.This is faster for Path & IPAddress then calling the constructor again.
For UUID it is also the only way to handle existing instances because
uuid.UUID()does not accept them as an argument.It is also possible to change the default value of
reuse_instancesvia theserialize&deserializedecorators.To not cause slowdowns when serializing or deserializing to json/msgpack/toml/yaml
reuse_instancesis always set toFalsethere, because we will never see existing instances there.That is the part where this pull request drifted into refactoring.
Since I had to add the
reuse_instancesargument at all these places, I also used the opportunity to remove some unused arguments (named&strict) and renamedasdicttoto_dictandastupletoto_tuple.These are breaking changes, but I felt it was worth the gained similarity between serialization and deserialization code.
In my option the public exposed functions are now also named more uniformly.
For msgpack external types I also changed the behaviour slightly. It is not required anymore that the dataclasses have a special meaning
_typeattribute.Instead
to_msgpack&from_msgpacksearch theext_dictfor the correct type or type code and also throw exceptions if they can not find them.I am sorry that this became so intertwined, I understand if you don't want to merge the breaking changes.
I tried to make the commits cherry-pickable so maybe you only want to use some commits.
During development, I also noticed that Unions do not work properly for these types.
I will make a separate pull request to fix that.
Finally, I have a question: What is the purpose of setting
SE_NAMEand why is it used inis_serializable?Could it be removed?
is_serializablecould useTO_ITERandTO_DICTfor the check likeis_deserializable.