Update enum flag handling

README.md

@@ -72,7 +72,7 @@ bazel run //circleci:workflows -- combine --output=/tmp/circleci.csv "${PWD}/dat
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).
 
 `--output OUTPUT`
 
@@ -136,7 +136,7 @@ bazel run //circleci:workflows -- fetch --output "${PWD}/data/circleci_workflows
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).
 
 `--workflow WORKFLOW`
 
@@ -212,7 +212,7 @@ bazel run //circleci:workflows -- fetch_details --input "${PWD}/data/circleci_wo
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).
 
 `--input INPUT`
 
@@ -321,7 +321,7 @@ bazel run //circleci:workflows -- request_branches
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).
 
 `--workflow WORKFLOW`
 
@@ -362,7 +362,7 @@ bazel run //circleci:workflows -- request_workflow --workflow_id <ID>
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).
 
 `--workflow_id WORKFLOW_ID`
 
@@ -402,4 +402,4 @@ bazel run //circleci:workflows -- request_workflows
 
 `--log_requests_details {['REQUEST', 'RESPONSE_TEXT', 'STATUS_CODE']}`
 
-> Comma separated list of LogRequestDetails.
+> Comma separated list of LogRequestDetails (default: [REQUEST]).

circleci/workflows_lib.py

@@ -128,7 +128,7 @@ def __init__(self) -> None:
             allow_empty=False,
             container_type=set,
             action=ActionEnumList,
-            help="Comma separated list of LogRequestDetails.",
+            help="Comma separated list of LogRequestDetails (default: [%(default_str)s]).",
         )
 
     def Prepare(self, argv: list[str]) -> None:

mbo/app/flags.py

@@ -72,16 +72,22 @@ class ActionEnumList(argparse.Action):
     * allow_empty:    If False (the default), then an empty list is NOT allowed.
     * container_type: The container type (e.g. list or set).
 
+    NOTE: The `type` is the exact Enum to work with. It will not be available as
+          a property of the action.
+
+    The string-ified default is available as `default_str` for consumprtion in
+    `help`.
+
     Example:
     ```
     parser.add_argument(
         "--myenum",
         default=[MyEnum.MY_DEFAULT],
         type=MyEnum,
         action=ActionEnumList,
-        allow_empty=False,
+        allow_empty=True,
         container_type=set,
-        help="Comma separated list of MyEnum values.",
+        help="Comma separated list of MyEnum values (default: %(default_str)s).",
     )
     args=parser.parse_args(["--nyenum", "my_default,my_other"])
     ```
@@ -91,68 +97,104 @@ class Choices:
         def __init__(self, action: argparse.Action):
             self._action: ActionEnumList = cast(ActionEnumList, action)
 
-        def choices(self) -> Iterable[str]:
-            return sorted(self._action._enum_type.__members__.keys())
-
-        def choices_list(self) -> str:
-            return ", ".join(self.choices())
-
         def __repr__(self) -> str:
-            return self.choices_list()
+            return self._action._choices_list()
 
         def __iter__(self):
-            yield self.choices()
+            yield self._action._choices()
 
-        def __contains__(self, value: str) -> bool:
-            if value == "":
-                if self._action._allow_empty:
-                    return True
-                raise argparse.ArgumentError(
-                    self._action,
-                    f"Empty value is not allowed, chose at least one of [{self.choices_list()}].",
-                )
-            for v in value.split(","):
-                v = v.strip()
-                if not v:
-                    raise argparse.ArgumentError(
-                        self._action,
-                        "Empty sub values are not allowed (that means values containing `,,`).",
-                    )
-                if v.upper() not in self.choices():
-                    raise argparse.ArgumentError(
-                        self._action,
-                        f"Sub value '{v}' is not a valid {self._action._enum_type} value, chose from [{self.choices_list()}].",
-                    )
+        def __contains__(self, value: Any) -> bool:
+            self._action._parse_list(value)
             return True
 
     def __init__(self, **kwargs):
-        self._enum_type = kwargs.pop("type", None)
+        self._enum_type = kwargs.pop("type", None)  # NOTE: Not actually setting `type`.
         self._allow_empty = kwargs.pop("allow_empty", False)
         self._container_type = kwargs.pop("container_type", list)
-
+        choices = kwargs.pop("choices", None)
+        kwargs.setdefault("choices", self.Choices(action=self))
+        super(ActionEnumList, self).__init__(**kwargs)
+        if choices:
+            raise argparse.ArgumentError(
+                self,
+                "Cannot (currently) restrict choices.",
+            )
         if self._enum_type is None or not issubclass(self._enum_type, Enum):
-            raise ValueError(
-                f"Type must be an Enum, provided type is '{self._enum_type}'."
+            raise argparse.ArgumentError(
+                self, f"Type must be an Enum, provided type is '{self._enum_type}'."
+            )
+        self.default = (
+            self._parse_list(self.default) if self.default else self._container_type()
+        )
+        self.default_str = ", ".join([str(v.name) for v in self.default])
+
+    def _choices(self) -> Iterable[str]:
+        return sorted(self._enum_type.__members__.keys())
+
+    def _choices_list(self) -> str:
+        return ", ".join(self._choices())
+
+    def _parse_list(self, values: str | Iterable[str | Enum] | None) -> Any:
+        if values:
+            if isinstance(values, str):
+                values = values.split(",")
+            elif isinstance(values, collections.abc.Iterable):
+                strs = []
+                enums = []
+                for v in values:
+                    if isinstance(v, str):
+                        strs.extend(v.split(","))
+                    elif isinstance(v, Enum):
+                        enums.append(v)
+                        strs.append(str(v))
+                    else:
+                        raise argparse.ArgumentError(
+                            self,
+                            f"Received bad sub value of type `{type(v)}` from values [{values}] of type `{type(values)}`, expected sub values of type `{self._enum_type}.",
+                        )
+                if enums and len(enums) == len(strs):
+                    return self._container_type(enums)
+                values = strs
+        if not values or [v for v in values if isinstance(v, str) and not v]:
+            if values:
+                v_str = "sub value"
+                given = f", given `{values}`"
+            else:
+                if self._allow_empty:
+                    return self._container_type()
+                v_str = "value"
+                given = ""
+            raise argparse.ArgumentError(
+                self,
+                f"Empty {v_str} is not allowed, chose at least one of [{self._choices_list()}]{given}.",
             )
+        return self._container_type(
+            [self._parse(v) for v in values if self._parse(v) is not None]
+        )
 
-        kwargs.setdefault("choices", self.Choices(action=self))
-        super(ActionEnumList, self).__init__(**kwargs)
+    def _parse(self, value: str | Enum) -> Enum | None:
+        if isinstance(value, self._enum_type):
+            return value
+        if not isinstance(value, str):
+            raise argparse.ArgumentError(
+                self,
+                f"Sub values must be of type `str` or `{self._enum_type}`, given `{type(value)}`.",
+            )
+        if not value:
+            raise argparse.ArgumentError(
+                self,
+                "Empty sub values are not allowed (that includes values containing `,,`).",
+            )
+        try:
+            return self._enum_type.__getitem__(value.strip().upper())
+        except KeyError as err:
+            raise argparse.ArgumentError(
+                self,
+                f"Sub value '{value}' is not a valid {self._enum_type} value, chose from [{self._choices_list()}].",
+            )
 
     def __call__(self, parser, namespace, values, option_string=None) -> None:
-        if isinstance(values, list):
-            values_str = ",".join(values)
-        elif isinstance(values, str):
-            values_str = values
-        else:
-            raise argparse.ArgumentError(self, "Unexpected value type {type(values)}.")
-        value = self._container_type(
-            [
-                self._enum_type.__getitem__(v.strip().upper())
-                for v in values_str.split(",")
-                if v
-            ]
-        )
-        setattr(namespace, self.dest, value)
+        setattr(namespace, self.dest, self._parse_list(values))
 
 
 def _MaybeMidnight(

mbo/app/flags_test.py

@@ -228,7 +228,7 @@ def FlagTest(self, test: FlagTestData) -> None:
                 input=["one"],
             ),
             FlagTestData(
-                test="Setting an empty vlaue requires `allow_empty=True`.",
+                test="Setting an empty value requires `allow_empty=True`.",
                 expected="argument --flag: Empty value is not allowed, chose at least one of [FOR, ONE, TRE, TWO].",
                 expected_error=argparse.ArgumentError,
                 action=ActionArgs(
@@ -240,7 +240,7 @@ def FlagTest(self, test: FlagTestData) -> None:
                 input=["--flag="],
             ),
             FlagTestData(
-                test="Setting an empty vlaue requires `allow_empty=True` (not False).",
+                test="Setting an empty value requires `allow_empty=True` (not False).",
                 expected="argument --flag: Empty value is not allowed, chose at least one of [FOR, ONE, TRE, TWO].",
                 expected_error=argparse.ArgumentError,
                 action=ActionArgs(
@@ -253,7 +253,7 @@ def FlagTest(self, test: FlagTestData) -> None:
                 input=["--flag="],
             ),
             FlagTestData(
-                test="Setting an empty vlaue requires with `allow_empty=True` works.",
+                test="Setting an empty default value requires `allow_empty=True`.",
                 expected=[],
                 expected_error=argparse.ArgumentError,
                 action=ActionArgs(
@@ -263,6 +263,19 @@ def FlagTest(self, test: FlagTestData) -> None:
                     action=mbo.app.flags.ActionEnumList,
                     allow_empty=True,
                 ),
+                input=[],
+            ),
+            FlagTestData(
+                test="Setting an empty value requires `allow_empty=True`.",
+                expected=[],
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    "--flag",
+                    type=TestEnum,
+                    default=[TestEnum.FOR],
+                    action=mbo.app.flags.ActionEnumList,
+                    allow_empty=True,
+                ),
                 input=["--flag="],
             ),
             FlagTestData(
@@ -277,8 +290,9 @@ def FlagTest(self, test: FlagTestData) -> None:
                 input=[],
             ),
             FlagTestData(
-                test="Default values work: They can even bypass the type.",
-                expected="Something else",
+                test="Default values cannot bypass the type.",
+                expected="argument --flag: Sub value 'Something else' is not a valid <enum 'TestEnum'> value, chose from [FOR, ONE, TRE, TWO].",
+                expected_error=argparse.ArgumentError,
                 action=ActionArgs(
                     "--flag",
                     type=TestEnum,
@@ -287,6 +301,17 @@ def FlagTest(self, test: FlagTestData) -> None:
                 ),
                 input=[],
             ),
+            FlagTestData(
+                test="Cannot (currently) restrict choices.",
+                expected="argument flag: Cannot (currently) restrict choices.",
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    type=TestEnum,
+                    action=mbo.app.flags.ActionEnumList,
+                    choices=[TestEnum.ONE, TestEnum.TWO],
+                ),
+                input=[],
+            ),
             FlagTestData(
                 test="Multile, possible repeated values and mixed case.",
                 expected=[TestEnum.TWO, TestEnum.ONE, TestEnum.TWO],
@@ -333,6 +358,69 @@ def FlagTest(self, test: FlagTestData) -> None:
                 ),
                 input=["two,for", "one,tre", "TWO"],
             ),
+            FlagTestData(
+                test="Repeated flag values still disallow empty values by default.",
+                expected="argument flag: Empty value is not allowed, chose at least one of [FOR, ONE, TRE, TWO].",
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    type=TestEnum,
+                    container_type=set,
+                    default=[TestEnum.TWO],
+                    action=mbo.app.flags.ActionEnumList,
+                ),
+                input=[""],
+            ),
+            FlagTestData(
+                test="Repeated flag values may allow empty values.",
+                expected=set(),
+                action=ActionArgs(
+                    type=TestEnum,
+                    container_type=set,
+                    default=[TestEnum.TWO],
+                    allow_empty=True,
+                    action=mbo.app.flags.ActionEnumList,
+                ),
+                input=[""],
+            ),
+            FlagTestData(
+                test="Repeated flag values may allow empty values but not empty sub values.",
+                expected="argument flag: Empty sub value is not allowed, chose at least one of [FOR, ONE, TRE, TWO], given `['one', '', 'tre']`.",
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    type=TestEnum,
+                    container_type=set,
+                    default=[TestEnum.TWO],
+                    # allow_empty=True,
+                    action=mbo.app.flags.ActionEnumList,
+                ),
+                input=["one,,tre"],
+            ),
+            FlagTestData(
+                test="Repeated flag values may not have empty sub values.",
+                expected="argument flag: Empty sub value is not allowed, chose at least one of [FOR, ONE, TRE, TWO], given `['one', '', 'tre']`.",
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    type=TestEnum,
+                    container_type=set,
+                    default=[TestEnum.TWO],
+                    allow_empty=False,
+                    action=mbo.app.flags.ActionEnumList,
+                ),
+                input=["one,,tre"],
+            ),
+            FlagTestData(
+                test="Repeated flag values may not have empty sub values even if empty values are allowed.",
+                expected="argument flag: Empty sub value is not allowed, chose at least one of [FOR, ONE, TRE, TWO], given `['one', '', 'tre']`.",
+                expected_error=argparse.ArgumentError,
+                action=ActionArgs(
+                    type=TestEnum,
+                    container_type=set,
+                    default=[TestEnum.TWO],
+                    allow_empty=True,
+                    action=mbo.app.flags.ActionEnumList,
+                ),
+                input=["one,,tre"],
+            ),
         ]
     )
     def test_EnumListAction(self, test: FlagTestData):