Repository: lebrice/SimpleParsing

Issue:
Confusing command line options for bool's with `default=True`
Having a boolean option with a default value of `True`:
```
from dataclasses import dataclass

from simple_parsing import ArgumentParser


@dataclass
class Args:
    enable_water: bool = True  # Do you want water?


parser = ArgumentParser()
parser.add_arguments(Args, dest="args")

print(parser.parse_args(["--enable_water"]))
# => Namespace(args=Args(enable_water=False))
```

Creates this very confusing behaviour that passing the option (without a boolean value) disables it, so passing `enable_water` actually causes `enable_water` to be set to False, which is not very intuitive.

Even though this behaviour corresponds to `store_false` in the classic python `ArgumentParser`, it would be nicer to make this more clear. For example by changing the command line option automatically to `--no_enable_water` (by prepending a `no_`) when the default is `False`, and then setting `enable_water` to False when the option `--no_enable_water` is passed.

Discussion:
Conchylicultor: Oups, sorry, it looks like VS code didn't pushed my changes.
I'll fix this tomorrow.

(Sorry, I'm not really used to the open source workflow anymore)
Conchylicultor: I pushed my changed. Sorry for the noise.

Let me know what you think
lebrice: Ok I'm happy with this. Thanks again @Conchylicultor. I'll update the branch and merge it.
I don't think backward-compatibility will be that big of an issue on this. Hopefully this doesn't come back to bite me later!
Conchylicultor: Fixed the test

> Does that change make sense?

The `[<>]` is a convention to specify that the `<>` is optional (so `--a [bool]` match `--a` or `--a True`).
Ideally, this should depend on `nargs`:

* `'?'`: `--a [bool]`
* `1`: `--a bool` 
* `'+'`: `--a bool [bool ...]`

But it's likely overengineered and I don't think this should block this PR
lebrice: Hey @Conchylicultor would you mind just correcting the expected strings that are causing the tests to fail?
I'll merge the PR once those pass.
Conchylicultor: Pushed a fix. Tests pass locally for me.

Patch:
diff --git a/examples/nesting/README.md b/examples/nesting/README.md
index f1ae397..2150500 100644
--- a/examples/nesting/README.md
+++ b/examples/nesting/README.md
@@ -119,40 +119,40 @@ usage: nesting_example.py [-h] [--batch_size int] [--optimizer str]
                           [--gender_loss_weight float]
                           [--age_loss_weight float]
                           [--max_number_of_likes int] [--embedding_dim int]
-                          [--shared_likes_embedding [str2bool]]
-                          [--use_custom_likes [str2bool]] [--gender.name str]
+                          [--shared_likes_embedding [bool]]
+                          [--use_custom_likes [bool]] [--gender.name str]
                           [--gender.num_layers int] [--gender.num_units int]
                           [--gender.activation str]
-                          [--gender.use_batchnorm [str2bool]]
-                          [--gender.use_dropout [str2bool]]
+                          [--gender.use_batchnorm [bool]]
+                          [--gender.use_dropout [bool]]
                           [--gender.dropout_rate float]
-                          [--gender.use_image_features [str2bool]]
-                          [--gender.use_likes [str2bool]]
+                          [--gender.use_image_features [bool]]
+                          [--gender.use_likes [bool]]
                           [--gender.l1_reg float] [--gender.l2_reg float]
-                          [--gender.embed_likes [str2bool]]
+                          [--gender.embed_likes [bool]]
                           [--age_group.name str] [--age_group.num_layers int]
                           [--age_group.num_units int]
                           [--age_group.activation str]
-                          [--age_group.use_batchnorm [str2bool]]
-                          [--age_group.use_dropout [str2bool]]
+                          [--age_group.use_batchnorm [bool]]
+                          [--age_group.use_dropout [bool]]
                           [--age_group.dropout_rate float]
-                          [--age_group.use_image_features [str2bool]]
-                          [--age_group.use_likes [str2bool]]
+                          [--age_group.use_image_features [bool]]
+                          [--age_group.use_likes [bool]]
                           [--age_group.l1_reg float]
                           [--age_group.l2_reg float]
-                          [--age_group.embed_likes [str2bool]]
+                          [--age_group.embed_likes [bool]]
                           [--personality.name str]
                           [--personality.num_layers int]
                           [--personality.num_units int]
                           [--personality.activation str]
-                          [--personality.use_batchnorm [str2bool]]
-                          [--personality.use_dropout [str2bool]]
+                          [--personality.use_batchnorm [bool]]
+                          [--personality.use_dropout [bool]]
                           [--personality.dropout_rate float]
-                          [--personality.use_image_features [str2bool]]
-                          [--personality.use_likes [str2bool]]
+                          [--personality.use_image_features [bool]]
+                          [--personality.use_likes [bool]]
                           [--personality.l1_reg float]
                           [--personality.l2_reg float]
-                          [--personality.embed_likes [str2bool]]
+                          [--personality.embed_likes [bool]]
 
 optional arguments:
   -h, --help            show this help message and exit
@@ -172,8 +172,8 @@ HyperParameters ['hparams']:
   --age_loss_weight float
   --max_number_of_likes int
   --embedding_dim int
-  --shared_likes_embedding [str2bool]
-  --use_custom_likes [str2bool]
+  --shared_likes_embedding [bool]
+  --use_custom_likes [bool]
                         Whether or not to use Rémi's better kept like pages
                         (default: True)
 
@@ -187,25 +187,25 @@ TaskHyperParameters ['hparams.gender']:
                         units per layer (default: 32)
   --gender.activation str
                         activation function (default: tanh)
-  --gender.use_batchnorm [str2bool]
+  --gender.use_batchnorm [bool]
                         whether or not to use batch normalization after each
                         dense layer (default: False)
-  --gender.use_dropout [str2bool]
+  --gender.use_dropout [bool]
                         whether or not to use dropout after each dense layer
                         (default: True)
   --gender.dropout_rate float
                         the dropout rate (default: 0.1)
-  --gender.use_image_features [str2bool]
+  --gender.use_image_features [bool]
                         whether or not image features should be used as input
                         (default: True)
-  --gender.use_likes [str2bool]
+  --gender.use_likes [bool]
                         whether or not 'likes' features should be used as input
                         (default: True)
   --gender.l1_reg float
                         L1 regularization coefficient (default: 0.005)
   --gender.l2_reg float
                         L2 regularization coefficient (default: 0.005)
-  --gender.embed_likes [str2bool]
+  --gender.embed_likes [bool]
                         Whether or not a task-specific Embedding layer should
                         be used on the 'likes' features. When set to 'True',
                         it is expected that there no shared embedding is used.
@@ -221,25 +221,25 @@ TaskHyperParameters ['hparams.age_group']:
                         units per layer (default: 64)
   --age_group.activation str
                         activation function (default: tanh)
-  --age_group.use_batchnorm [str2bool]
+  --age_group.use_batchnorm [bool]
                         whether or not to use batch normalization after each
                         dense layer (default: False)
-  --age_group.use_dropout [str2bool]
+  --age_group.use_dropout [bool]
                         whether or not to use dropout after each dense layer
                         (default: True)
   --age_group.dropout_rate float
                         the dropout rate (default: 0.1)
-  --age_group.use_image_features [str2bool]
+  --age_group.use_image_features [bool]
                         whether or not image features should be used as input
                         (default: True)
-  --age_group.use_likes [str2bool]
+  --age_group.use_likes [bool]
                         whether or not 'likes' features should be used as input
                         (default: True)
   --age_group.l1_reg float
                         L1 regularization coefficient (default: 0.005)
   --age_group.l2_reg float
                         L2 regularization coefficient (default: 0.005)
-  --age_group.embed_likes [str2bool]
+  --age_group.embed_likes [bool]
                         Whether or not a task-specific Embedding layer should
                         be used on the 'likes' features. When set to 'True',
                         it is expected that there no shared embedding is used.
@@ -256,25 +256,25 @@ TaskHyperParameters ['hparams.personality']:
                         units per layer (default: 8)
   --personality.activation str
                         activation function (default: tanh)
-  --personality.use_batchnorm [str2bool]
+  --personality.use_batchnorm [bool]
                         whether or not to use batch normalization after each
                         dense layer (default: False)
-  --personality.use_dropout [str2bool]
+  --personality.use_dropout [bool]
                         whether or not to use dropout after each dense layer
                         (default: True)
   --personality.dropout_rate float
                         the dropout rate (default: 0.1)
-  --personality.use_image_features [str2bool]
+  --personality.use_image_features [bool]
                         whether or not image features should be used as input
                         (default: False)
-  --personality.use_likes [str2bool]
+  --personality.use_likes [bool]
                         whether or not 'likes' features should be used as input
                         (default: False)
   --personality.l1_reg float
                         L1 regularization coefficient (default: 0.005)
   --personality.l2_reg float
                         L2 regularization coefficient (default: 0.005)
-  --personality.embed_likes [str2bool]
+  --personality.embed_likes [bool]
                         Whether or not a task-specific Embedding layer should
                         be used on the 'likes' features. When set to 'True',
                         it is expected that there no shared embedding is used.
diff --git a/simple_parsing/wrappers/field_wrapper.py b/simple_parsing/wrappers/field_wrapper.py
index bb00dec..b76f231 100644
--- a/simple_parsing/wrappers/field_wrapper.py
+++ b/simple_parsing/wrappers/field_wrapper.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import argparse
+from collections.abc import Callable, Iterable, Sequence
 import dataclasses
 import inspect
 import sys
@@ -377,10 +378,7 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                 _arg_options["type"] = type_fn
 
         elif utils.is_bool(self.type):
-            _arg_options["type"] = utils.str2bool
-            _arg_options["type"].__name__ = "bool"
-            _arg_options["nargs"] = "?"
-
+            _arg_options["action"] = _BooleanOptionalAction
         else:
             # "Plain" / simple argument.
             # For the metavar, use a custom passed value, if present, else do
@@ -489,16 +487,6 @@ class FieldWrapper(Wrapper[dataclasses.Field]):
                 return tuple(raw_parsed_value)
 
         elif self.is_bool:
-            # print(self.name, raw_parsed_value)
-            if self.dest_field:
-                # TODO: This isn't used anywhere, what was the idea again?
-                # other_default = self.dest_field.field.metadata.get("_original_default")
-                pass
-                # print(other_default)
-
-            if raw_parsed_value is None and self.default is not None:
-                logger.debug("value is None, returning opposite of the default")
-                return not self.default
             return raw_parsed_value
 
         elif self.is_list:
@@ -1093,3 +1081,87 @@ def only_keep_action_args(options: dict[str, Any], action: str | Any) -> dict[st
         logger.debug(f"Kept options: \t{kept_options.keys()}")
         logger.debug(f"Removed options: \t{deleted_options.keys()}")
     return kept_options
+
+
+_BOOL_NO_PREFIX = "--no"
+
+
+class _BooleanOptionalAction(argparse.Action):
+    """Similar to `argparse.BooleanOptionalAction`.
+
+    * Support custom prefix (for `absl.flags` compatibility)
+    * Accept `--flag=true` value
+    * Support `nargs`
+    * Support Python 3.8
+
+    """
+
+    def __init__(
+        self,
+        option_strings: Sequence[str],
+        dest: str,
+        default: bool | None = None,
+        type: Callable[[str], Any] | argparse.FileType | None = None,
+        choices: Iterable[Any] | None = None,
+        required: bool = False,
+        help: str | None = None,
+        metavar: str | tuple[str, ...] | None = None,
+        nargs: int | str | None = '?',
+    ):
+        if nargs is None:
+            nargs = '?'
+        if not (isinstance(nargs, int) or nargs in {'?', '*', '+'}):
+            raise ValueError(f'nargs={nargs!r} not supported for bools')
+
+        # Do not add `--no` when `nargs >= 1`
+        if nargs in {'+', '*'} or (isinstance(nargs, int) and nargs >= 1):
+            new_option_strings = list(option_strings)
+        else:
+            new_option_strings = []
+            for option_string in option_strings:
+                new_option_strings.append(option_string)
+
+                if option_string.startswith("--"):
+                    negative_option_string = _BOOL_NO_PREFIX + option_string[2:]
+                    new_option_strings.append(negative_option_string)
+
+        super().__init__(
+            option_strings=new_option_strings,
+            dest=dest,
+            nargs=nargs,
+            default=default,
+            type=type,
+            choices=choices,
+            required=required,
+            help=help,
+            metavar=metavar or "bool",
+        )
+
+    def __call__(
+        self,
+        parser: ArgumentParser,
+        namespace: argparse.Namespace,
+        values: Any,
+        option_string: str | None = None,
+    ):
+        if option_string not in self.option_strings:
+            return
+
+        is_neg = option_string.startswith(_BOOL_NO_PREFIX)
+        if values is None:  # --my_flag / --nomy_flag
+            bool_value = not is_neg
+        elif is_neg:  # Cannot set `--nomy_flag=True/False`
+            parser.exit(
+                message=f"{_BOOL_NO_PREFIX} cannot be used with value (Got: {option_string}={values})"
+            )
+        elif isinstance(values, str):  # --my_flag true
+            bool_value = utils.str2bool(values)
+        elif isinstance(values, list):  # --my_flag true true false
+            bool_value = [utils.str2bool(v) for v in values]
+        else:
+            raise ValueError(f"Unsuported value for {option_string}: {values!r}")
+
+        setattr(namespace, self.dest, bool_value)
+
+    def format_usage(self):
+        return " | ".join(self.option_strings)


Test patch:
diff --git a/test/test_bools.py b/test/test_bools.py
index a7096da..5cfed08 100644
--- a/test/test_bools.py
+++ b/test/test_bools.py
@@ -1,5 +1,8 @@
 from dataclasses import dataclass
 
+import pytest
+
+from simple_parsing import helpers
 from .testutils import TestSetup
 
 
@@ -18,26 +21,85 @@ class Flags(TestSetup):
     c: bool = False  # optional flag 'c'.
 
 
-def test_bool_attributes_work():
-    ext = Base.setup("--a 5 --f")
-    assert ext.f is True
-
-    ext = Base.setup("--a 5")
-    assert ext.f is False
+@pytest.mark.parametrize(
+    "flag,expected_f",
+    [
+        ("--a 5", False),
+        ("--a 5 --f", True),
+        ("--a 5 --nof", False),
+        ("--a 5 --f=true", True),
+        ("--a 5 --f true", True),
+        ("--a 5 --f True", True),
+        ("--a 5 --f=false", False),
+        ("--a 5 --f false", False),
+        ("--a 5 --f False", False),
+        ("--a 5 --f --f false", False),
+    ],
+)
+def test_bool_base_work(flag: str, expected_f: bool):
+    assert Base.setup(flag).f is expected_f
 
-    true_strings = ["True", "true"]
-    for s in true_strings:
-        ext = Base.setup(f"--a 5 --f {s}")
-        assert ext.f is True
+@pytest.mark.parametrize(
+    "flag,a,b,c",
+    [
+        ("--a", True, True, False),
+        ("--a true --b --c", True, True, True),
+        ("--a true --noc --b --c --noc", True, True, False),
+        ("--noa --b false --noc", False, False, False),
+    ],
+)
+def test_bool_flags_work(flag: str, a: bool, b: bool, c: bool):
+    flags = Flags.setup(flag)
+    assert flags.a is a
+    assert flags.b is b
+    assert flags.c is c
 
-    false_strings = ["False", "false"]
-    for s in false_strings:
-        ext = Base.setup(f"--a 5 --f {s}")
-        assert ext.f is False
 
+@pytest.mark.parametrize(
+    "flag, nargs, a",
+    [
+        # By default, support both --noflag and --flag=false
+        ("--a", '?', True),
+        ("--noa", '?', False),
+        ("--a true", '?', True),
+        ("--a true false", '?', SystemExit('unrecognized arguments')),
+        # `nargs=None` is like `nargs='?'`
+        ("--a", None, True),
+        ("--noa", None, False),
+        ("--a true", None, True),
+        ("--a true false", None, SystemExit('unrecognized arguments')),
+        # 1 argument explicitly required
+        ("--a", 1, SystemExit('expected 1 argument')),
+        ("--noa", 1, SystemExit('the following arguments are required')),
+        ("--a=true", 1, [True]),
+        ("--a true false", 1, SystemExit('unrecognized arguments')),
+        # 2 argument explicitly required
+        ("--a", 2, SystemExit('expected 2 argument')),
+        ("--noa", 2, SystemExit('the following arguments are required')),
+        ("--a=true", 2, SystemExit('expected 2 argument')),
+        ("--a true false", 2, [True, False]),
+        # 1+ argument explicitly required
+        ("--a", '+', SystemExit('expected at least one argument')),
+        ("--noa", '+', SystemExit('the following arguments are required')),
+        ("--a=true", '+', [True]),
+        ("--a true false", '+', [True, False]),
+        # 0 or 1+ argument explicitly required
+        ("--a", '*', []),
+        ("--noa", '*', SystemExit('unrecognized arguments')),
+        ("--a=true", '*', [True]),
+        ("--a true false", '*', [True, False]),
+    ],
+)
+def test_bool_nargs(flag, nargs, a, capsys: pytest.CaptureFixture):
+    @dataclass
+    class MyClass(TestSetup):
+        """Some test class"""
+        a: bool = helpers.field(nargs=nargs)
 
-def test_bool_flags_work():
-    flags = Flags.setup("--a true --b --c")
-    assert flags.a is True
-    assert flags.b is False
-    assert flags.c is True
+    if isinstance(a, SystemExit):
+        with pytest.raises(SystemExit):
+            MyClass.setup(flag)
+        assert str(a) in capsys.readouterr().err
+    else:
+        flags = MyClass.setup(flag)
+        assert flags.a == a