lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
428 stars 52 forks source link

Support serialization for tuples with ellipsis #59

Closed yycho0108 closed 3 years ago

yycho0108 commented 3 years ago

Hi again! This is Yoonyoung (Jamie) Cho.

I've been using SimpleParsing quite a lot recently, and I've come across a use case for serialization for variable-length homogeneous tuples. There was a TODO (# TODO: support the Ellipsis?) and I needed this capability, so I figured I should create a PR for this feature. Hopefully this is useful.

Description

Currently, decode_tuple() cannot handle cases such as Tuple[int,...]. There's a check in parse_tuple() that deals with ellipses, but similar fix was not adopted for serialization (decoding such tuples) - I tried to follow the same style as the existing function, which looks to be more robust than simply checking for tuple_item_types[0], although I agree with the comments that in general the only allowable form of such tuples is Tuple[T, ...] (other uses of ellipses would result in a TypeError such as Tuple[t0, t1, ...]: each t must be a type. Got Ellipsis.).

I also added a test in utils/test_serialization.py for simply checking the equivalence of the dataclass instance after saving to and loading from the serialized string.

Pytest

$ pytest utils/test_serialization.py
=========================================== test session starts ===========================================
platform linux -- Python 3.6.9, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /home/jamiecho/Repos/Ravel/SimpleParsing
plugins: forked-1.3.0, xdist-2.2.0, cov-2.11.1, env-0.6.2
collected 19 items                                                                                        

utils/test_serialization.py ...................                                                     [100%]

=========================================== 19 passed in 0.08s ============================================

Hopefully this is useful!

lebrice commented 3 years ago

Thanks a lot @yycho0108 , this is really neat! I'll create a new release (v0.0.15.post2) to include your changes!

lebrice commented 3 years ago

Would you mind rebasing your branch on master before I merge this?

yycho0108 commented 3 years ago

Yep, rebased just now. Thanks!