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
|
Is this still being actively worked on or should I close this? |
Hi @shyuep Thanks for the heads up on this. Completely forgot about it. I guess that the question was more rhetorical and that "actively worked on" was ironic since it's been 5 years :) I'll see quickly if we still have a use case for this (as I forgot about it and did not pursue, probably not, at least not urgently). I'll close it myself end of next week if there is no need to keep it. Best |
shyuep
left a comment
shyuep
left a comment
There was a problem hiding this comment.
Automated review by Claude (on behalf of @shyuep)
This PR adds useful serialization support for tuples, namedtuples, sets, and OrderedDicts, but has several blocking issues that need to be resolved before it can be merged.
Critical — Python 3.9 incompatibility:
The entire is_NamedTuple detection and validate_NamedTuple rely on _field_types, which was deprecated in Python 3.8 and removed in Python 3.9. As a result, is_NamedTuple will always return False on Python 3.9+, and validate_NamedTuple will raise AttributeError. The correct modern equivalent is __annotations__. The distinguishing logic between collections.namedtuple and typing.NamedTuple needs to be rewritten for Python 3.9+.
Title vs. implementation mismatch:
The PR title and description mention frozenset support, but frozenset is not handled anywhere in the diff.
Stale version guards:
Monty already requires Python ≥ 3.8, so all the sys.version_info >= (3, 6) / >= (3, 7) checks are dead code and should be removed.
Test style:
Using exec() to define classes in tests (to work around Python version restrictions) is now unnecessary given the minimum Python version. These should be replaced with regular class definitions.
Merge conflicts:
The PR has merge conflicts against the current master and will need to be rebased.
The overall approach is sound — please update to handle modern Python and rebase onto master.
Generated by Claude Code
2 participants
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.