lebrice / SimpleParsing

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

Optional List/Tuple only accepts first value #47

Closed PleasantD closed 3 years ago

PleasantD commented 3 years ago

Describe the bug Specifying a List or Tuple field as optional only accepts the first value.

To Reproduce

from dataclasses import dataclass
from typing import Optional, Tuple
from simple_parsing import ArgumentParser

@dataclass
class MyConfig:
    values: Optional[Tuple[int, int]]

parser = ArgumentParser()
parser.add_arguments(MyConfig, dest="cfg")
args_none, _ = parser.parse_known_args([])
args_values, extra = parser.parse_known_args(["--values", "3", "4"])

Expected behavior args_values.cfg.values should be the tuple (3,4) and extra should be [] But instead args_values.cfg.values is 3 and extra is ['4']

Additionally there should be parse error if the number of items for a fixed-length tuple does not match the definition. IE, specifying too few or too many values should generate an error if the tuple is of a fixed length.

lebrice commented 3 years ago

Thanks for posting @PleasantD! Indeed, this seems related to #42, it appears like the parsing of Tuple fields is bugged.

Additionally there should be parse error if the number of items for a fixed-length tuple does not match the definition. IE, specifying too few or too many values should generate an error if the tuple is of a fixed length.

The problem here seems to me more related to the Optional annotation, as it's not easy to determine the right value to pass to the nargs argument of add_argument in this case.. Would you have an opinion on which value of 'nargs' would be appropriate in this case?

I'm fairly certain that the parsing of fixed length tuples (a.k.a fields annotated with Tuple[int, str, float] etc.) works correctly, at least based on the following tests: https://github.com/lebrice/SimpleParsing/blob/master/test/test_tuples.py

Would you mind taking a quick glance at these tests, and letting me know what you think?

Thanks again for posting!

rggjan commented 3 years ago

Just stumbled on the same issue. And yes it seems foo: Optional[Tuple[str, str]] is broken, while foo:Tuple[str, str] works fine. Same thing for Lists like foo: Optional[List[str]].

Shouldn't it be something like:

This would even allow specifying (and differentiating) between [] and None for optional lists, like: