Repository: anntzer/defopt

Issue:
Should `Optional[Tuple[type1]]` be supported
I noticed that while `Optional[SomeContainer[type1]]` is supported for most of the containers defopt supports (i.e., List, Sequence, Collection, Iterable), it is not supported for Tuple. I would expect `Optional[Tuple[type1]]` to behave similarly to other containers, in that it is treated as simply `Tuple[type1]`. I was able to add support for this locally within a couple of minutes, but figured I would check to make sure it wasn't deliberately unsupported before pushing the fix.

And thanks again for actively maintaining defopt! I use it quite a bit in my job.

Discussion:
anntzer: Mostly looks good (in light of https://github.com/anntzer/defopt/issues/115#issuecomment-1159549912), just one comment.

Patch:
diff --git a/lib/defopt.py b/lib/defopt.py
index 921a182..a1c2884 100644
--- a/lib/defopt.py
+++ b/lib/defopt.py
@@ -71,6 +71,12 @@ __all__ = ['run', 'signature', 'bind', 'bind_known']
 
 _PARAM_TYPES = ['param', 'parameter', 'arg', 'argument', 'key', 'keyword']
 _TYPE_NAMES = ['type', 'kwtype']
+_LIST_TYPES = [
+    list,
+    collections.abc.Iterable,
+    getattr(collections.abc, "Collection", object()),
+    collections.abc.Sequence,
+]
 
 _Doc = namedtuple('_Doc', ('first_line', 'text', 'params', 'raises'))
 _Param = namedtuple('_Param', ('text', 'type'))
@@ -506,6 +512,7 @@ def _populate_parser(func, parser, parsers, short, cli_options,
         if ((cli_options == 'kwonly' or
              (param.default is param.empty and cli_options == 'has_default'))
             and not _is_list_like(param.annotation)
+            and not _is_optional_list_like(param.annotation)
             and param.kind != param.KEYWORD_ONLY)}
     if short is None:
         count_initials = Counter(name[0] for name in sig.parameters
@@ -528,8 +535,8 @@ def _populate_parser(func, parser, parsers, short, cli_options,
         required = not hasdefault and param.kind != param.VAR_POSITIONAL
         positional = name in positionals
         if type_ in [bool, typing.Optional[bool]] and not positional:
-            action = ('store_true' if no_negated_flags and
-                      default in [False, None]
+            action = ('store_true'
+                      if no_negated_flags and default in [False, None]
                       else _BooleanOptionalAction)  # --name/--no-name
             actions.append(_add_argument(
                 parser, name, short, action=action, default=default,
@@ -549,6 +556,16 @@ def _populate_parser(func, parser, parsers, short, cli_options,
                 kwargs['default'] = _DefaultList()
         else:
             kwargs['required'] = required
+
+        # If the type is an Optional container, extract only the container.
+        union_args = _ti_get_args(type_) if _is_union_type(type_) else []
+        if any(_is_container(subtype) for subtype in union_args):
+            non_none = [arg for arg in union_args if arg is not type(None)]
+            if len(non_none) != 1:
+                raise ValueError('unsupported union including container type: '
+                                 '{}'.format(type_))
+            type_, = non_none
+
         if _is_list_like(type_):
             type_, = _ti_get_args(type_)
             kwargs['nargs'] = '*'
@@ -563,14 +580,18 @@ def _populate_parser(func, parser, parsers, short, cli_options,
             kwargs['metavar'] = '{' + ','.join(type_.__members__) + '}'
         elif _ti_get_origin(type_) is tuple:
             member_types = _ti_get_args(type_)
+            num_members = len(member_types)
             # Variable-length tuples of homogenous type are specified like
             # Tuple[int, ...]
-            if len(member_types) == 2 and member_types[1] is Ellipsis:
-                kwargs['nargs'] = "*"
+            if num_members == 2 and member_types[1] is Ellipsis:
+                kwargs['nargs'] = '*'
                 kwargs['action'] = _make_store_tuple_action_class(
                     tuple, member_types, parsers, is_variable_length=True)
+            elif type(None) in union_args and parsers.get(type(None)):
+                raise ValueError('Optional tuples and NoneType parsers cannot '
+                                 'be used together due to ambiguity')
             else:
-                kwargs['nargs'] = len(member_types)
+                kwargs['nargs'] = num_members
                 kwargs['action'] = _make_store_tuple_action_class(
                     tuple, member_types, parsers)
         elif (isinstance(type_, type) and issubclass(type_, tuple)
@@ -645,12 +666,22 @@ def _update_help_string(action, *, show_defaults, show_types):
 
 
 def _is_list_like(type_):
-    return _ti_get_origin(type_) in [
-        list,
-        collections.abc.Iterable,
-        getattr(collections.abc, 'Collection', object()),
-        collections.abc.Sequence,
-    ]
+    return _ti_get_origin(type_) in _LIST_TYPES
+
+
+def _is_container(type_):
+    return _ti_get_origin(type_) in {*_LIST_TYPES, tuple}
+
+
+def _is_union_type(type_):
+    return _ti_get_origin(type_) in {Union, getattr(types, 'UnionType', '')}
+
+
+def _is_optional_list_like(type_):
+    # Assume a union with a list subtype is actually Optional[list[...]]
+    # because this condition is enforced in other places
+    return (_is_union_type(type_)
+            and any(_is_list_like(subtype) for subtype in _ti_get_args(type_)))
 
 
 def _get_type(func, name):
@@ -692,18 +723,12 @@ def _get_type_from_doc(name, globalns):
     if ' or ' in name:
         subtypes = [_get_type_from_doc(part, globalns)
                     for part in name.split(' or ')]
-        if any(map(_is_list_like, subtypes)):
+        if any(map(_is_list_like, subtypes)) and None not in subtypes:
             raise ValueError(
                 'unsupported union including container type: {}'.format(name))
         return Union[tuple(subtype for subtype in subtypes)]
-    # Support for sphinx-specific "list[type]", "tuple[type]" syntax; only
-    # needed for Py<3.9.
-    # (This intentionally won't catch `List` or `typing.List`.)
-    match = re.match(r'(list|tuple)\[([\w\.]+)\]', name)
-    if match:
-        container, type_ = match.groups()
-        container = {'list': List, 'tuple': Tuple}[container]
-        return container[eval(type_, globalns)]
+    if sys.version_info < (3, 9):  # Support "list[type]", "tuple[type]".
+        globalns = {**globalns, 'tuple': Tuple, 'list': List}
     return _get_type_from_hint(eval(name, globalns))
 
 
@@ -711,15 +736,6 @@ def _get_type_from_hint(hint):
     if _is_list_like(hint):
         [type_] = _ti_get_args(hint)
         return List[type_]
-    elif _ti_get_origin(hint) is Union:
-        args = _ti_get_args(hint)
-        if any(_is_list_like(subtype) for subtype in args):
-            non_none = [arg for arg in args if arg is not type(None)]
-            if len(non_none) != 1:
-                raise ValueError(
-                    'unsupported union including container type: {}'
-                    .format(hint))
-            return non_none[0]
     return hint
 
 
@@ -975,7 +991,7 @@ def _get_parser(type_, parsers):
         parser = _make_enum_parser(type_)
     elif _is_constructible_from_str(type_):
         parser = functools.partial(type_)
-    elif _ti_get_origin(type_) in [Union, getattr(types, "UnionType", "")]:
+    elif _is_union_type(type_):
         args = _ti_get_args(type_)
         if type(None) in args:
             # If None is in the Union, parse it first.  This only matters if


Test patch:
diff --git a/test_defopt.py b/test_defopt.py
index fcef426..9e3d67a 100644
--- a/test_defopt.py
+++ b/test_defopt.py
@@ -766,6 +766,93 @@ class TestUnion(unittest.TestCase):
         self.assertEqual(defopt.run(main, argv=['--foo', '1']), [1])
 
 
+class TestOptional(unittest.TestCase):
+    def test_optional_hint_list(self):
+        def main(op: typing.Optional[typing.List[str]]): return op
+        self.assertEqual(defopt.run(main, argv=['--op', '1', '2']), ['1', '2'])
+
+    def test_optional_hint_tuple(self):
+        def main(op: typing.Optional[typing.Tuple[int]]): return op
+        self.assertEqual(defopt.run(main, argv=['1']), (1,))
+
+    def test_optional_doc_list(self):
+        def main(op):
+            """:param typing.Optional[typing.List[int]] op: op"""
+            return op
+        self.assertEqual(defopt.run(main, argv=['--op', '1', '2']), [1, 2])
+
+    def test_optional_doc_tuple(self):
+        def main(op):
+            """:param typing.Optional[typing.Tuple[str]] op: op"""
+            return op
+        self.assertEqual(defopt.run(main, argv=['1']), ('1',))
+
+    def test_union_operator_hint_list(self):
+        if not hasattr(types, "UnionType"):
+            raise unittest.SkipTest("A|B-style unions not supported")
+        def main(op: typing.List[str] | None): return op
+        self.assertEqual(defopt.run(main, argv=['--op', '1', '2']), ['1', '2'])
+
+    def test_union_operator_hint_tuple(self):
+        if not hasattr(types, "UnionType"):
+            raise unittest.SkipTest("A|B-style unions not supported")
+        def main(op: typing.Tuple[int] | None): return op
+        self.assertEqual(defopt.run(main, argv=['1']), (1,))
+
+    def test_union_operator_doc_list(self):
+        if not hasattr(types, "UnionType"):
+            raise unittest.SkipTest("A|B-style unions not supported")
+        def main(op):
+            """:param list[int]|None op: op"""
+            return op
+        self.assertEqual(defopt.run(main, argv=['--op', '1', '2']), [1, 2])
+
+    def test_union_operator_doc_one_item_tuple(self):
+        if not hasattr(types, "UnionType"):
+            raise unittest.SkipTest("A|B-style unions not supported")
+        def main(op):
+            """:param tuple[int]|None op: op"""
+            return op
+        self.assertEqual(defopt.run(main, argv=['1']), (1,))
+
+    def test_union_operator_doc_multiple_item_tuple(self):
+        if not hasattr(types, "UnionType"):
+            raise unittest.SkipTest("A|B-style unions not supported")
+        def main(op):
+            """:param tuple[str,str]|None op: op"""
+            return op
+        self.assertEqual(defopt.run(main, argv=['a', 'b']), ('a', 'b'))
+
+    def test_multiple_item_optional_tuple_none_parser(self):
+        def main(op):
+            """:param typing.Optional[typing.Tuple[int, int]] op: op"""
+            return op
+        def _parse_none(i):
+            if i.lower() == 'none':
+                return None
+            else:
+                raise ValueError('{} is not a valid None string'.format(i))
+        with self.assertRaises(ValueError):
+            defopt.run(main, argv=['1', '2'],
+                       parsers={type(None): _parse_none})
+
+    def test_one_item_optional_tuple_none_parser(self):
+        # As long as there is not support for trying the NoneType parser
+        # before the tuple parser in the case of a one-item optional tuple,
+        # this case should also raise an error. See comments on GH115 for more
+        # details.
+        def main(op):
+            """:param typing.Optional[typing.Tuple[str]] op: op"""
+            return op
+        def _parse_none(i):
+            if i.lower() == 'none':
+                return None
+            else:
+                raise ValueError('{} is not a valid None string'.format(i))
+        with self.assertRaises(ValueError):
+            defopt.run(main, argv=['1'], parsers={type(None): _parse_none})
+
+
 class TestLiteral(unittest.TestCase):
     def test_literal(self):
         def main(foo):
@@ -1041,11 +1128,6 @@ class TestAnnotations(unittest.TestCase):
         self.assertEqual(defopt._get_type_from_hint(typing.Sequence[int]),
                          typing.List[int])
 
-    def test_optional(self):
-        self.assertEqual(
-            defopt._get_type_from_hint(typing.Optional[typing.List[int]]),
-            typing.List[int])
-
     def test_conflicting(self):
         def foo(bar: int):
             """:type bar: float"""