Skip to content

Commit

Permalink
Use OptionInfo type for option registration args/kwargs. (#21766)
Browse files Browse the repository at this point in the history
The options registration system has for a long time passed around
the naked args and kwargs used at registration time.

However the more recent implementation of class field-based
declarative options introduced an `OptionsInfo` dataclass to
encapsulate that data. 

This PR spreads the use of `OptionsInfo` into the options
registration code. Advantages include:

- More succinct code.
- Proper type annotations.
- No confusion about when to use `*` or `**`.

Note that this change renames `OptionsInfo` to `OptionInfo`, 
since it represents the registration of a single option.
It also renames its fields to `args` and `kwargs`, since that
is the usage in most of the related code. Also, the name `flag_options`
was doubly confusing: A) A flag is just one aspect of an option, and 
B) "options" is overloaded. This refers the knobs you can set when 
registering an option, so using the word "options" for it is
brain-hurting.
I considered "knobs", but figured "kwargs" was just as good, and already
in use.
  • Loading branch information
benjyw authored Dec 17, 2024
1 parent fddc1d3 commit e82a4ff
Show file tree
Hide file tree
Showing 12 changed files with 115 additions and 112 deletions.
12 changes: 6 additions & 6 deletions src/python/pants/core/util_rules/environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from pants.engine.unions import UnionRule
from pants.option import custom_types
from pants.option.global_options import GlobalOptions
from pants.option.option_types import DictOption, OptionsInfo, collect_options_info
from pants.option.option_types import DictOption, OptionInfo, collect_options_info
from pants.option.subsystem import Subsystem
from pants.util.enums import match
from pants.util.frozendict import FrozenDict
Expand Down Expand Up @@ -1160,12 +1160,12 @@ def option_field_name_for(flag_names: Sequence[str]) -> str:

def _add_option_field_for(
env_aware_t: type[Subsystem.EnvironmentAware],
option: OptionsInfo,
option: OptionInfo,
) -> Iterable[UnionRule]:
option_type: type = option.flag_options["type"]
option_type: type = option.kwargs["type"]
scope = env_aware_t.subsystem.options_scope

snake_name = option_field_name_for(option.flag_names)
snake_name = option_field_name_for(option.args)

# Note that there is not presently good support for enum options. `str`-backed enums should
# be easy enough to add though...
Expand All @@ -1180,7 +1180,7 @@ def _add_option_field_for(
"to a `Field` subtype that supports your option's value type."
)
else:
member_type = option.flag_options["member_type"]
member_type = option.kwargs["member_type"]
try:
field_type = _LIST_OPTIONS[member_type]
except KeyError:
Expand All @@ -1203,7 +1203,7 @@ class OptionField(field_type, _EnvironmentSensitiveOptionFieldMixin): # type: i
"environment target is active."
)
subsystem = env_aware_t
option_name = option.flag_names[0]
option_name = option.args[0]

setattr(OptionField, "__qualname__", f"{option_type.__qualname__}.{OptionField.__name__}")

Expand Down
51 changes: 26 additions & 25 deletions src/python/pants/help/help_info_extracter.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
from pants.engine.target import Field, RegisteredTargetTypes, StringField, Target, TargetGenerator
from pants.engine.unions import UnionMembership, UnionRule, is_union
from pants.option.native_options import NativeOptionParser, parse_dest
from pants.option.option_types import OptionInfo
from pants.option.option_util import is_dict_option, is_list_option
from pants.option.options import Options
from pants.option.ranked_value import Rank, RankedValue
Expand Down Expand Up @@ -1014,10 +1015,8 @@ def get_option_scope_help_info(
basic_options = []
advanced_options = []
deprecated_options = []
for args, kwargs in registrar.option_registrations_iter():
derivation = native_parser.get_derivation(
scope=registrar.scope, registration_args=args, registration_kwargs=kwargs
)
for option_info in registrar.option_registrations_iter():
derivation = native_parser.get_derivation(registrar.scope, option_info)
# Massage the derivation structure returned by the NativeOptionParser into an
# OptionValueHistory as returned by the legacy parser.
# TODO: Once we get rid of the legacy parser we can probably simplify by
Expand All @@ -1027,20 +1026,20 @@ def get_option_scope_help_info(
# Adding this constant, empty history entry is silly, but it appears in the
# legacy parser's results as an implementation artifact, and we want to be
# consistent with its tests until we get rid of it.
is_list = kwargs.get("type") == list
is_dict = kwargs.get("type") == dict
is_list = option_info.kwargs.get("type") == list
is_dict = option_info.kwargs.get("type") == dict
empty_val: list | dict | None = [] if is_list else {} if is_dict else None
empty_details = "" if (is_list or is_dict) else None
ranked_values.append(RankedValue(Rank.NONE, empty_val, empty_details))

for value, rank, details in derivation:
ranked_values.append(RankedValue(rank, value, details or empty_details))
history = OptionValueHistory(tuple(ranked_values))
ohi = self.get_option_help_info(args, kwargs)
ohi = self.get_option_help_info(option_info)
ohi = dataclasses.replace(ohi, value_history=history)
if ohi.deprecation_active:
deprecated_options.append(ohi)
elif kwargs.get("advanced"):
elif option_info.kwargs.get("advanced"):
advanced_options.append(ohi)
else:
basic_options.append(ohi)
Expand All @@ -1056,13 +1055,13 @@ def get_option_scope_help_info(
deprecated=tuple(deprecated_options),
)

def get_option_help_info(self, args, kwargs):
def get_option_help_info(self, option_info: OptionInfo) -> OptionHelpInfo:
"""Returns an OptionHelpInfo for the option registered with the given (args, kwargs)."""
display_args = []
scoped_cmd_line_args = []
unscoped_cmd_line_args = []

for arg in args:
for arg in option_info.args:
is_short_arg = len(arg) == 2
unscoped_cmd_line_args.append(arg)
if self._scope_prefix:
Expand All @@ -1071,7 +1070,7 @@ def get_option_help_info(self, args, kwargs):
scoped_arg = arg
scoped_cmd_line_args.append(scoped_arg)

if OptionRegistrar.is_bool(kwargs):
if OptionRegistrar.is_bool(option_info.kwargs):
if is_short_arg:
display_args.append(scoped_arg)
else:
Expand All @@ -1080,18 +1079,18 @@ def get_option_help_info(self, args, kwargs):
scoped_cmd_line_args.append(f"--no-{sa_2}")
display_args.append(f"--[no-]{sa_2}")
else:
metavar = self.compute_metavar(kwargs)
metavar = self.compute_metavar(option_info.kwargs)
separator = "" if is_short_arg else "="
display_args.append(f"{scoped_arg}{separator}{metavar}")
if kwargs.get("passthrough"):
type_str = self.stringify_type(kwargs.get("member_type", str))
if option_info.kwargs.get("passthrough"):
type_str = self.stringify_type(option_info.kwargs.get("member_type", str))
display_args.append(f"... -- [{type_str} [{type_str} [...]]]")

typ = kwargs.get("type", str)
default = self.compute_default(**kwargs)
help_msg = kwargs.get("help", "No help available.")
deprecation_start_version = kwargs.get("deprecation_start_version")
removal_version = kwargs.get("removal_version")
typ = option_info.kwargs.get("type", str)
default = self.compute_default(**option_info.kwargs)
help_msg = option_info.kwargs.get("help", "No help available.")
deprecation_start_version = option_info.kwargs.get("deprecation_start_version")
removal_version = option_info.kwargs.get("removal_version")
deprecation_active = removal_version is not None and deprecated.is_deprecation_active(
deprecation_start_version
)
Expand All @@ -1106,10 +1105,10 @@ def get_option_help_info(self, args, kwargs):
deprecated_message = (
f"{message_start}, {deprecated_tense} removed in version: {removal_version}."
)
removal_hint = kwargs.get("removal_hint")
choices = self.compute_choices(kwargs)
removal_hint = option_info.kwargs.get("removal_hint")
choices = self.compute_choices(option_info.kwargs)

dest = parse_dest(*args, **kwargs)
dest = parse_dest(option_info)
udest = dest.upper()
if self._scope == GLOBAL_SCOPE:
# Global options have 2-3 env var variants, e.g., --pants-workdir can be
Expand All @@ -1122,9 +1121,11 @@ def get_option_help_info(self, args, kwargs):
else:
env_var = f"PANTS_{_ENV_SANITIZER_RE.sub('_', self._scope.upper())}_{udest}"

target_field_name = f"{self._scope_prefix}_{option_field_name_for(args)}".replace("-", "_")
environment_aware = kwargs.get("environment_aware") is True
fromfile = kwargs.get("fromfile", False)
target_field_name = (
f"{self._scope_prefix}_{option_field_name_for(option_info.args)}".replace("-", "_")
)
environment_aware = option_info.kwargs.get("environment_aware") is True
fromfile = option_info.kwargs.get("fromfile", False)

ret = OptionHelpInfo(
display_args=tuple(display_args),
Expand Down
22 changes: 11 additions & 11 deletions src/python/pants/help/help_info_extracter_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from pants.option.config import Config
from pants.option.global_options import GlobalOptions, LogLevelOption
from pants.option.native_options import NativeOptionParser
from pants.option.option_types import BoolOption, IntListOption, StrListOption
from pants.option.option_types import BoolOption, IntListOption, OptionInfo, StrListOption
from pants.option.options import Options
from pants.option.ranked_value import Rank
from pants.option.registrar import OptionRegistrar
Expand All @@ -35,7 +35,7 @@ def test_global_scope():
def do_test(args, kwargs, expected_display_args, expected_scoped_cmd_line_args):
# The scoped and unscoped args are the same in global scope.
expected_unscoped_cmd_line_args = expected_scoped_cmd_line_args
ohi = HelpInfoExtracter("").get_option_help_info(args, kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(args, kwargs))
assert tuple(expected_display_args) == ohi.display_args
assert tuple(expected_scoped_cmd_line_args) == ohi.scoped_cmd_line_args
assert tuple(expected_unscoped_cmd_line_args) == ohi.unscoped_cmd_line_args
Expand Down Expand Up @@ -81,7 +81,7 @@ def do_test(
expected_scoped_cmd_line_args,
expected_unscoped_cmd_line_args,
):
ohi = HelpInfoExtracter("bar.baz").get_option_help_info(args, kwargs)
ohi = HelpInfoExtracter("bar.baz").get_option_help_info(OptionInfo(args, kwargs))
assert tuple(expected_display_args) == ohi.display_args
assert tuple(expected_scoped_cmd_line_args) == ohi.scoped_cmd_line_args
assert tuple(expected_unscoped_cmd_line_args) == ohi.unscoped_cmd_line_args
Expand Down Expand Up @@ -142,56 +142,56 @@ def do_test(expected_default: Optional[Any], **kwargs):

def test_deprecated():
kwargs = {"removal_version": "999.99.9", "removal_hint": "do not use this"}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert "999.99.9" == ohi.removal_version
assert "do not use this" == ohi.removal_hint
assert ohi.deprecated_message is not None
assert ohi.deprecation_active


def test_not_deprecated():
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], {})
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), {}))
assert ohi.removal_version is None
assert not ohi.deprecation_active


def test_deprecation_start_version_past():
kwargs = {"deprecation_start_version": "1.0.0", "removal_version": "999.99.9"}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert "999.99.9" == ohi.removal_version
assert ohi.deprecation_active


def test_deprecation_start_version_future():
kwargs = {"deprecation_start_version": "999.99.8", "removal_version": "999.99.9"}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert "999.99.9" == ohi.removal_version
assert not ohi.deprecation_active


def test_passthrough():
kwargs = {"passthrough": True, "type": list, "member_type": str}
ohi = HelpInfoExtracter("").get_option_help_info(["--thing"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--thing",), kwargs))
assert 2 == len(ohi.display_args)
assert any(args.startswith("--thing") for args in ohi.display_args)
assert any(args.startswith("... -- ") for args in ohi.display_args)


def test_choices() -> None:
kwargs = {"choices": ["info", "debug"]}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert ohi.choices == ("info", "debug")


def test_choices_enum() -> None:
kwargs = {"type": LogLevelSimple}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert ohi.choices == ("info", "debug")


def test_list_of_enum() -> None:
kwargs = {"type": list, "member_type": LogLevelSimple}
ohi = HelpInfoExtracter("").get_option_help_info(["--foo"], kwargs)
ohi = HelpInfoExtracter("").get_option_help_info(OptionInfo(("--foo",), kwargs))
assert ohi.choices == ("info", "debug")


Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -2259,11 +2259,11 @@ def create(cls, GlobalOptionsType: type[GlobalOptions]) -> GlobalOptionsFlags:
short_flags = set()

for options_info in collect_options_info(BootstrapOptions):
for flag in options_info.flag_names:
for flag in options_info.args:
flags.add(flag)
if len(flag) == 2:
short_flags.add(flag)
elif options_info.flag_options.get("type") == bool:
elif options_info.kwargs.get("type") == bool:
flags.add(f"--no-{flag[2:]}")

return cls(FrozenOrderedSet(flags), FrozenOrderedSet(short_flags))
Expand Down
37 changes: 20 additions & 17 deletions src/python/pants/option/native_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,15 @@
from pants.option.config import ConfigSource
from pants.option.custom_types import _flatten_shlexed_list, dir_option, file_option, shell_str
from pants.option.errors import BooleanOptionNameWithNo, OptionsError, ParseError
from pants.option.option_types import OptionInfo
from pants.option.ranked_value import Rank
from pants.option.scope import GLOBAL_SCOPE
from pants.util.strutil import get_strict_env, softwrap

logger = logging.getLogger()


def parse_dest(*args: str, **kwargs) -> str:
def parse_dest(option_info: OptionInfo) -> str:
"""Return the dest for an option registration.
If an explicit `dest` is specified, returns that and otherwise derives a default from the
Expand All @@ -34,12 +35,12 @@ def parse_dest(*args: str, **kwargs) -> str:
- The key in the config file.
- Computing the name of the env var used to set the option name.
"""
dest = kwargs.get("dest")
dest = option_info.kwargs.get("dest")
if dest:
return str(dest)
# No explicit dest, so compute one based on the first long arg, or the short arg
# if that's all there is.
arg = next((a for a in args if a.startswith("--")), args[0])
arg = next((a for a in option_info.args if a.startswith("--")), option_info.args[0])
return arg.lstrip("-").replace("-", "_")


Expand Down Expand Up @@ -120,30 +121,32 @@ def with_derivation(self) -> NativeOptionParser:
known_scopes_to_flags=self._known_scopes_to_flags,
)

def get_value(self, *, scope, registration_args, registration_kwargs) -> Tuple[Any, Rank]:
val, rank, _ = self._get_value_and_derivation(scope, registration_args, registration_kwargs)
def get_value(self, *, scope: str, option_info: OptionInfo) -> Tuple[Any, Rank]:
val, rank, _ = self._get_value_and_derivation(scope, option_info)
return (val, rank)

def get_derivation(
self, *, scope, registration_args, registration_kwargs
self,
scope: str,
option_info: OptionInfo,
) -> list[Tuple[Any, Rank, Optional[str]]]:
_, _, derivation = self._get_value_and_derivation(
scope, registration_args, registration_kwargs
)
_, _, derivation = self._get_value_and_derivation(scope, option_info)
return derivation

def _get_value_and_derivation(
self, scope, registration_args, registration_kwargs
self,
scope: str,
option_info: OptionInfo,
) -> Tuple[Any, Rank, list[Tuple[Any, Rank, Optional[str]]]]:
return self._get(
scope=scope,
dest=parse_dest(*registration_args, **registration_kwargs),
flags=registration_args,
default=registration_kwargs.get("default"),
option_type=registration_kwargs.get("type"),
member_type=registration_kwargs.get("member_type"),
choices=registration_kwargs.get("choices"),
passthrough=registration_kwargs.get("passthrough"),
dest=parse_dest(option_info),
flags=option_info.args,
default=option_info.kwargs.get("default"),
option_type=option_info.kwargs.get("type"),
member_type=option_info.kwargs.get("member_type"),
choices=option_info.kwargs.get("choices"),
passthrough=option_info.kwargs.get("passthrough"),
)

def _get(
Expand Down
Loading

0 comments on commit e82a4ff

Please sign in to comment.