Skip to content

Commit

Permalink
Update enum flag handling: (#12)
Browse files Browse the repository at this point in the history 
* For workflows_lib.py flag `--log_requests_details` show the default in its help.
* Improve enum flag parsing:
  * improve type handling,
  * clarify and add error messages, and
  * add tests.
  • Loading branch information
@helly25
helly25 authored Sep 3, 2024
1 parent 59291cb commit ad73a60
Show file tree
Hide file tree
Showing 4 changed files with 192 additions and 62 deletions.
12 changes: 6 additions & 6 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down Expand Up @@ -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`

Expand Down Expand Up @@ -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`

Expand Down Expand Up @@ -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`

Expand Down Expand Up @@ -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`

Expand Down Expand Up @@ -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]).
2 changes: 1 addition & 1 deletion circleci/workflows_lib.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
142 changes: 92 additions & 50 deletions mbo/app/flags.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
```
Expand All @@ -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(
Expand Down
98 changes: 93 additions & 5 deletions mbo/app/flags_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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,
Expand All @@ -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],
Expand Down Expand Up @@ -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):
Expand Down

0 comments on commit ad73a60

Please sign in to comment.