WIP Added support for tuples, namedtuples (both from collections and typing), sets, frozensets and OrderedDict's in MSONable, MontyEncoder and MontyDecoder #100
Conversation
…in collections.py. Unit test for above method.
Unit tests for tuple and namedtuple serialization.
Unit tests for tuple, namedtuple and OrderedDict serialization. Works with MSONable class as well as with MontyEncoder and MontyDecoder.
values were not possible.
… typing.NamedTuple. Unit tests for is_NamedTuple.
| a_nt = namedtuple('a', ['x', 'y', 'z']) | ||
| a1 = a_nt(1, 2, 3) | ||
| assert a1 == (1, 2, 3) | ||
| assert is_namedtuple(a1) is True |
There was a problem hiding this comment.
I don't think you need "is True". The assert tests whether it is True.
Similarly, 3 lines later you just need "assert not is_named_tuple..."
There was a problem hiding this comment.
It is actually not exactly equivalent. The "is True" also checks that the returned value is a bool and not for example an empty list vs a non empty list. If is_namedtuple(a1) were to return, e.g. ["hello"] (or to return [] in the "False" case), "assert is_namedtuple(a1)" would also pass while the API would have changed. I would keep the "is True" if that's ok for you. Same for 3 lines after (and also in quite a lot of other places).
There was a problem hiding this comment.
Yes, I know it is not exactly equivalent, but we need to think of typical use cases. When people use that method, they are not going to check that it is a boolean type. They will merely check that it is a "True-like" value. E.g., if is_namedtuple(a1). That's the Pythonic way of doing this.
There was a problem hiding this comment.
Of course, users will use it this way. My point is that this method is supposed (from its docstring/name) to return a bool indicating whether it is a namedtuple or not. Why should we allow any new development/change to return something else (e.g. the string "Yes it is a namedtuple" for the True case and "" for the False case) ? This unit test is just preventing that. Anyway if you prefer to remove the "is True", I can do it. For me the pythonic way applies for the code itself, but the test code can deviate from that for the reason above.
| def test_tuple_serialization(self): | ||
| a = GoodMSONClass(a=1, b=tuple(), c=1) | ||
| afromdict = GoodMSONClass.from_dict(a.as_dict()) | ||
| assert type(a.b) is tuple |
There was a problem hiding this comment.
I think isinstance is a better approach. This applies to all instances where you use type(X) is or == something.
There was a problem hiding this comment.
isinstance checks that the instance is a tuple or a subclass of tuple. I think it is more safe to use type(a.b) is tuple as it would prevent someone (a weirdo btw ;-) to change the MSONable as_dict and/or from_dict to return a subclass of tuple when deserializing. What do you think ?
There was a problem hiding this comment.
I agree. But this is a test, not the actual code. So here, it suffices to make sure it is an instance of tuple.
There was a problem hiding this comment.
If the user serializes a tuple, he expects to get back exactly a tuple when deserializing. Using isinstance in the test allows to change that. I could modify the MSONable from_dict to return a subclass of a tuple instead of a real tuple.
…have the correct type. Unit tests for validate_NamedTuple.
…field types for the different fields.
…field types for the different fields.
…edtuples, etc ... in order to avoid potential clashes with the builtins package.
davidwaroquiers
changed the title
davidwaroquiers
changed the title
Summary
Solves #89
TODO
Might want to add support for other objects, e.g. Data classes and possibly others.
Not sure whether the last try-except in the default method of MontyEncoder is still necessary as the as_dict is now directly performed (recursively) at the beginning of the encode method of MontyEncoder.