Repository: anntzer/defopt

Issue:
Feature Request: Make Optional[bool] still act as a flag
Consider the code below.  I'd like to have the argument `skip` still act as a normal boolean flag and then program would only get None if neither `--skip` nor `--no-skip` is provided.  The idea here is that if the user explicitly sets this flag, then it should be followed.  But if they don't set the flag explicitly (so it is None), then the CLI would have some logic to decide if the default value would be `True` or `False`, potentially depending on some other input, like `loops` in this dummy example.

```python
from typing import Optional

import defopt


def test(*, loops: int = 5, skip: Optional[bool] = None):
    """This is a dummy function.

    Parameters
    ----------
    skip
        Skip printing something.
    """
    print(f'loops = {loops}')
    print(f'skip  = {skip}')
    _skip = skip if skip is not None else loops > 10
    if _skip:
        print(f'skipping loop printing')
    for i in range(loops):
        if not _skip:
            print(f'long loop iteration: {i+1}')

if __name__ == '__main__':
    defopt.run(test)
```

But this is now recognized as a variable input instead of a flag.

```console
$ python dummy.py --help
usage: dummy.py [-h] [-l LOOPS] [-s SKIP]

This is a dummy function.

optional arguments:
  -h, --help            show this help message and exit
  -l LOOPS, --loops LOOPS
                        (default: 5)
  -s SKIP, --skip SKIP  Skip printing something.
                        (default: None)
nox > Session dummy was successful.

```

So specifying nothing works:

```console
$ python dummy.py
loops = 5
skip  = None
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 5
```

Specifying 0 or 1 as the input works:

```console
$ python dummy.py --skip 1
loops = 5
skip  = True
skipping loop printing
$ python dummy.py --skip 0
loops = 5
skip  = False
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 
```

But of course using just `--skip` alone (or `--no-skip`) doesn't work.  Note also that, as far as I can tell, you can't manually specify the value of `None` either:

```console
$ python dummy.py --skip None
usage: dummy.py [-h] [-l LOOPS] [-s SKIP]
dummy.py: error: argument -s/--skip: invalid typing.Optional[bool] value: 'None'
```

What I would like, is for this to be treated as a flag still.  This would be a breaking change since now using the example of `python dummy.py --skip 1` would no longer work as instead it would just be `python dummy.py --skip` or `python dummy.py --no-skip`.  I personally can't imagine any scenario in which using `--skip 1` and `--skip 0` is better than `--skip` and `--no-skip`.

A PR to consider for this is incoming.

Discussion:
anntzer: Good catch, this seems like a complete regression introduced by https://github.com/anntzer/defopt/commit/ac198e8 (which was implemented in response to #87, which seems like a reasonable request in general).

I think this just needs a changelog entry, and a bit of text expanding on the block in features.rst by that commit to explain the special-casing of bools (no need for a long rationale, just say that bools behave differently wrt Optional).
Spectre5: One more thing to consider as well is what to do if an `Optional[bool]` is used with no negated flags.  Then you effectively cannot get the False value, only True or None.  I suppose that could still be desired if the CLI function is also used outside of defopt, but it seems unlikely that it would generally be the intent?
Spectre5: > Extra brownie points if you additionally check what happens if a parser is provided for bool (e.g. one could decide instead to parse 1/t/true as True and 0/f/false as False), and, if that's indeed supported, have Optional[bool] behave as for normal types.

Can you explain this part more?  What exactly do you mean here, to remove them as flags and instead use `--skip t` or `--skip f`, for example?  I'd personally rather have `--skip` and `--no-skip` for an optional bool, but I'm probably misunderstanding your intent here.
Spectre5: I think that just works:

```python
from typing import Optional

import defopt


def _parse_none(string):
    if string.lower() == 'none':
        return None
    else:
        raise ValueError('{} is not a valid None string'.format(string))


def test(loops: int = 5, skip: Optional[bool] = None):
    """This is a dummy function.

    Parameters
    ----------
    skip
        Skip printing something.
    """
    print(f'loops = {loops}')
    print(f'skip  = {skip}')
    _skip = skip if skip is not None else loops > 10
    if _skip:
        print(f'skipping loop printing')
    for i in range(loops):
        if not _skip:
            print(f'long loop iteration: {i+1}')

if __name__ == '__main__':
    defopt.run(test, parsers={type(None): _parse_none})
```

```console
$ python dummy.py 5 t
loops = 5
skip  = True
$ python dummy.py 5 None
loops = 5
skip  = None
long loop iteration: 1
long loop iteration: 2
long loop iteration: 3
long loop iteration: 4
long loop iteration: 5

```
Spectre5: And when used as a keyword only argument, I don't think you even can specify a bool parser for either a bool or Optional[bool] type as it would get consumed before the parser was accessed here:

https://github.com/anntzer/defopt/blob/0e9032c5cdfc8a9ca9cfcd6a64317ad8d84e21bf/lib/defopt.py#L448
anntzer: OK, I think this looks reasonable (just needs changelog entries and updating features.rst).  Let's treat this as a bugfix (as noted above) and think about interactions with no_negated_flags and custom bool parsers another time.
Spectre5: Ok, I will make those updates soon.

As for the interactions with the parsers, can you describe more about what your idea or ideal scenario is there?  I won't incorporate it in this PR but will keep it in mind if I have some time in the future to work it.  To keep that discussion separate, maybe open it as a new issue?
anntzer: (I'm off until new year, will continue this discussion after that. Happy holidays :)) 
Spectre5: Sounds good.  Check out the doc updates (features and changelog) and let me know how you'd like it edited.  For the changelog, I moved all previous entries to 6.2.0 and this is the only new one under the next section now.  I assume this is correct, but please double check it!
Spectre5: Done.  Just curious, why don't you use the "squash and merge" option in GitHub when merging it?
Spectre5: Oops, restored this branch
anntzer: Mostly out of habit, but also to allow the contributor to still split the work in a couple of logical commits if the patch is really large and separate commits make sense (and they want to do so).

Patch:
diff --git a/doc/source/changelog.rst b/doc/source/changelog.rst
index 202d75b..27b518a 100644
--- a/doc/source/changelog.rst
+++ b/doc/source/changelog.rst
@@ -1,6 +1,10 @@
 Changelog
 =========
 
+next
+----
+* Fixed `typing.Optional[bool]` to still be treated as a flag.
+
 6.2.0 (2021-11-24)
 ------------------
 * Bumped docutils dependency to >=0.12, for setuptools compatibility.
diff --git a/doc/source/features.rst b/doc/source/features.rst
index ac9c411..b543bca 100644
--- a/doc/source/features.rst
+++ b/doc/source/features.rst
@@ -243,6 +243,12 @@ order" rule given above, a parser for ``type(None)`` will always be tried
 first; this is so that e.g. ``Optional[str]`` can parse some user-chosen values
 as ``None`` and the others as ``str``.
 
+``typing.Optional[bool]`` is treated separately, as a special case, to still
+act as a boolean flag.  Defining a default value of ``None`` for the argument
+will result in receiving ``None`` if the option is not specified on the command
+line and either ``True`` or ``False`` if one of the two boolean flags are
+provided.
+
 Collection types are not supported in unions; e.g. ``Union[List[type1]]``
 is not supported (with the exception of ``Optional[List[type1]]``, which is
 *always* equivalent to ``List[type1]``).
diff --git a/lib/defopt.py b/lib/defopt.py
index ae56b5c..f434fd4 100644
--- a/lib/defopt.py
+++ b/lib/defopt.py
@@ -450,8 +450,9 @@ def _populate_parser(func, parser, parsers, short, cli_options,
         default = param.default if hasdefault else SUPPRESS
         required = not hasdefault and param.kind != param.VAR_POSITIONAL
         positional = name in positionals
-        if type_ == bool and not positional:
-            action = ('store_true' if no_negated_flags and default is False
+        if type_ in [bool, typing.Optional[bool]] and not positional:
+            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,


Test patch:
diff --git a/test_defopt.py b/test_defopt.py
index c167ebd..3173ace 100644
--- a/test_defopt.py
+++ b/test_defopt.py
@@ -405,6 +405,82 @@ class TestParsers(unittest.TestCase):
         with self.assertRaises(SystemExit):
             defopt.run(main, argv=[])
 
+    def test_bool_optional(self):
+        def main(foo=None):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        def _parse_none(i):
+            if i.lower() == 'none':
+                return None
+            else:
+                raise ValueError('{} is not a valid None string'.format(i))
+        self.assertIs(defopt.run(main, argv=['1'],
+                      parsers={type(None): _parse_none}), True)
+        self.assertIs(defopt.run(main, argv=['0'], 
+                      parsers={type(None): _parse_none}), False)
+        self.assertIs(defopt.run(main, argv=['None'],
+                      parsers={type(None): _parse_none}), None)
+        self.assertIs(defopt.run(main, argv=[],
+                      parsers={type(None): _parse_none}), None)
+
+    def test_bool_optional_keyword_none(self):
+        def main(*, foo=None):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        self.assertIs(defopt.run(main, argv=['--foo']), True)
+        self.assertIs(defopt.run(main, argv=['--no-foo']), False) 
+        self.assertIs(defopt.run(main, argv=[]), None)
+
+    def test_bool_optional_keyword_true(self):
+        def main(*, foo=True):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        # cannot get a None value in this case from the CLI
+        self.assertIs(defopt.run(main, argv=['--foo']), True)
+        self.assertIs(defopt.run(main, argv=['--no-foo']), False) 
+        self.assertIs(defopt.run(main, argv=[]), True)
+
+    def test_bool_optional_keyword_false(self):
+        def main(*, foo=False):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        self.assertIs(defopt.run(main, argv=['--foo']), True)
+        self.assertIs(defopt.run(main, argv=['--no-foo']), False) 
+        self.assertIs(defopt.run(main, argv=[]), False)
+
+    def test_bool_optional_keyword_none_no_negated_flags(self):
+        def main(*, foo=None):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        self.assertIs(defopt.run(main, argv=['--foo'], no_negated_flags=True),
+                      True)
+        with self.assertRaises(SystemExit):
+            self.assertIs(defopt.run(main, argv=['--no-foo'],
+                          no_negated_flags=True), False)
+        self.assertIs(defopt.run(main, argv=[], no_negated_flags=True), None)
+
+    def test_bool_optional_keyword_true_no_negated_flags(self):
+        def main(*, foo=True):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        self.assertIs(defopt.run(main, argv=['--foo'], no_negated_flags=True),
+                      True)
+        # negated flag is still added, else foo could only be True
+        self.assertIs(defopt.run(main, argv=['--no-foo'],
+                      no_negated_flags=True), False)
+        self.assertIs(defopt.run(main, argv=[], no_negated_flags=True), True)
+
+    def test_bool_optional_keyword_false_no_negated_flags(self):
+        def main(*, foo=False):
+            """:type foo: typing.Optional[bool]"""
+            return foo
+        self.assertIs(defopt.run(main, argv=['--foo'], no_negated_flags=True),
+                      True)
+        with self.assertRaises(SystemExit):
+            self.assertIs(defopt.run(main, argv=['--no-foo'],
+                          no_negated_flags=True), False)
+        self.assertIs(defopt.run(main, argv=[], no_negated_flags=True), False)
+
     def test_bool_list(self):
         def main(foo):
             """:type foo: list[bool]"""