Skip to content

Commit

Permalink
Fix dynamic defaults (pantsbuild#14637)
Browse files Browse the repository at this point in the history
After doing the work to switch the remaining subsystem options to the new style I hit some mypy errors. This fixes those (and adds tests to ensure we don't hit them again).

[ci skip-rust]
[ci skip-build-wheels]
  • Loading branch information
thejcannon authored Feb 26, 2022
1 parent 43085e1 commit b6db571
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 15 deletions.
24 changes: 11 additions & 13 deletions src/python/pants/option/option_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,17 @@ class OptionsInfo:
# subsytem type and returning a value. E.g. `prop = Option(..., default=lambda cls: cls.default)`.
# This is necessary to support "base" subsystem types which are subclassed with more specific
# values.
_DynamicDefaultT = Callable[[_SubsystemType], _DefaultT]
# NB: Marking this as `Callable[[_SubsystemType], _DefaultT]` will upset mypy at the call site
# because mypy won't be able to have enough info to deduce the correct type.
_DynamicDefaultT = Callable[[_SubsystemType], Any]
# The type of the `default` parameter for each option.
_MaybeDynamicT = Union[_DynamicDefaultT[_DefaultT], _DefaultT]
_MaybeDynamicT = Union[_DynamicDefaultT, _DefaultT]
# The type of the `help` parameter for each option.
_HelpT = _MaybeDynamicT[str]


def _eval_maybe_dynamic(val: _MaybeDynamicT[_DefaultT], subsystem_cls: _SubsystemType) -> _DefaultT:
return val(subsystem_cls) if inspect.isfunction(val) else val # type: ignore[operator,return-value]
return val(subsystem_cls) if inspect.isfunction(val) else val # type: ignore[operator,return-value,no-any-return]


class _OptionBase(Generic[_OptT, _DefaultT]):
Expand All @@ -51,7 +53,7 @@ class _OptionBase(Generic[_OptT, _DefaultT]):
"""

_flag_names: tuple[str, ...]
_default: _DefaultT
_default: _MaybeDynamicT[_DefaultT]
_help: _HelpT
_extra_kwargs: dict[str, Any]

Expand All @@ -60,7 +62,7 @@ class _OptionBase(Generic[_OptT, _DefaultT]):
def __new__(
cls,
*flag_names: str,
default: _DefaultT,
default: _MaybeDynamicT[_DefaultT],
help: _HelpT,
# Additional bells/whistles
advanced: bool | None = None,
Expand Down Expand Up @@ -150,7 +152,7 @@ class _ListOptionBase(
def __new__(
cls,
*flag_names: str,
default: list[_ListMemberT] = [],
default: _MaybeDynamicT[list[_ListMemberT]] = [],
help: _HelpT,
# Additional bells/whistles
advanced: bool | None = None,
Expand Down Expand Up @@ -382,9 +384,7 @@ def __new__(
cls,
*flag_names: str,
enum_type: type[_EnumT],
# NB: Marking this as `_DynamicDefaultT[_EnumT]` will upset mypy at the call site because
# mypy won't be able to have enough info to deduce the correct type.
default: _DynamicDefaultT[Any],
default: _DynamicDefaultT,
help: _HelpT,
# Additional bells/whistles
advanced: bool | None = None,
Expand Down Expand Up @@ -509,9 +509,7 @@ def __new__(
cls,
*flag_names: str,
enum_type: type[_EnumT],
# NB: Marking this as `_DynamicDefaultT[list[_EnumT]]` will upset mypy at the call site
# because mypy won't be able to have enough info to deduce the correct type.
default: _DynamicDefaultT[Any],
default: _DynamicDefaultT,
help: _HelpT,
# Additional bells/whistles
advanced: bool | None = ...,
Expand Down Expand Up @@ -627,7 +625,7 @@ class DictOption(_OptionBase["dict[str, _ValueT]", "dict[str, _ValueT]"], Generi
def __new__(
cls,
*flag_names,
default: dict[str, _ValueT] = {},
default: _MaybeDynamicT[dict[str, _ValueT]] = {},
help,
advanced: bool | None = None,
daemon: bool | None = None,
Expand Down
10 changes: 8 additions & 2 deletions src/python/pants/option/option_types_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ def __init__(self):
dyn_enum_list_prop = EnumListOption(
"--dyn-enum-list-opt",
enum_type=MyEnum,
default=lambda cls: [cls.dyn_default],
default=lambda cls: cls.dyn_default_list,
help=lambda cls: f"{cls.dyn_help}",
)
defaultless_enum_list_prop = EnumListOption(
Expand All @@ -146,6 +146,7 @@ def __init__(self):
class MySubsystem(MyBaseSubsystem):
dyn_help = "Dynamic Help"
dyn_default = MyEnum.Val1
dyn_default_list = [MyEnum.Val1]

register = Mock()
MySubsystem.register_options(register)
Expand Down Expand Up @@ -265,6 +266,8 @@ def __init__(self):
file_list_opt = FileListOption("--opt", help="")
shellstr_list_opt = ShellStrListOption("--opt", help="")
memorysize_list_opt = MemorySizeListOption("--opt", help="")
# And just test one dynamic default
dyn_str_list_opt = StrListOption("--opt", default=lambda cls: cls.default, help="")

# Enum opts
enum_opt = EnumOption("--opt", default=MyEnum.Val1, help="")
Expand All @@ -277,7 +280,7 @@ def __init__(self):
enum_list_opt1 = EnumListOption("--opt", default=[MyEnum.Val1], help="")
enum_list_opt2 = EnumListOption("--opt", enum_type=MyEnum, help="")
dyn_enum_list_opt = EnumListOption(
"--opt", enum_type=MyEnum, default=lambda cls: [cls.default], help=""
"--opt", enum_type=MyEnum, default=lambda cls: cls.default_list, help=""
)
# mypy correctly complains about needing a type annotation
enum_list_bad_opt = EnumListOption("--opt", default=[], help="") # type: ignore[var-annotated]
Expand All @@ -291,6 +294,7 @@ def __init__(self):
dict_opt5 = DictOption("--opt", default=dict(key="val"), help="")
dict_opt6 = DictOption("--opt", default=dict(key=1), help="")
dict_opt7 = DictOption("--opt", default=dict(key1=1, key2="str"), help="")
dyn_dict_opt = DictOption[str]("--opt", lambda cls: cls.default, help="")

my_subsystem = MySubsystem()
if TYPE_CHECKING:
Expand Down Expand Up @@ -322,6 +326,7 @@ def __init__(self):
assert_type["tuple[str, ...]"](my_subsystem.file_list_opt)
assert_type["tuple[str, ...]"](my_subsystem.shellstr_list_opt)
assert_type["tuple[int, ...]"](my_subsystem.memorysize_list_opt)
assert_type["tuple[str, ...]"](my_subsystem.dyn_str_list_opt)

assert_type["MyEnum"](my_subsystem.enum_opt)
assert_type["MyEnum | None"](my_subsystem.optional_enum_opt)
Expand All @@ -339,3 +344,4 @@ def __init__(self):
assert_type["dict[str, str]"](my_subsystem.dict_opt5)
assert_type["dict[str, int]"](my_subsystem.dict_opt6)
assert_type["dict[str, object]"](my_subsystem.dict_opt7)
assert_type["dict[str, str]"](my_subsystem.dyn_dict_opt)

0 comments on commit b6db571

Please sign in to comment.