Move option registration validation earlier #21645
Conversation
| @@ -23,7 +23,7 @@ | |||
| logger = logging.getLogger() | |||
|
|
|||
|
|
|||
| def parse_dest(*args, **kwargs): | |||
| def parse_dest(*args: str, **kwargs) -> str: | |||
There was a problem hiding this comment.
Turns out this annotation means "all *args must be strings" to mypy, which is good.
| @@ -60,7 +60,7 @@ def test_format_help_choices(self) -> None: | |||
| @classmethod | |||
| def _get_parser(cls) -> Tuple[Parser, NativeOptionParser]: | |||
| return Parser( | |||
| scope_info=GlobalOptions.get_scope_info(), | |||
| scope=GlobalOptions.options_scope, | |||
There was a problem hiding this comment.
We don't need "ScopeInfo" in some places now, and hopefully in all places soon...
| @@ -1014,7 +1014,7 @@ def test_parse_dest() -> None: | |||
|
|
|||
|
|
|||
| def test_validation() -> None: | |||
| def assertError(expected_error, *args, **kwargs): | |||
| def assert_error(expected_error, *args, **kwargs): | |||
There was a problem hiding this comment.
Just a rename for convention.
| @@ -1046,7 +1046,7 @@ def test_shadowing() -> None: | |||
| args=["./pants"], | |||
| ) | |||
| options.register("", "--opt1") | |||
| options.register("foo", "-o", "--opt2") | |||
| options.register("foo", "--opt2") | |||
There was a problem hiding this comment.
This was a test that didn't adhere to our own validation rules, but skirted them since we don't parse in this test.
| @@ -213,9 +180,9 @@ def normalize_kwargs(orig_args, orig_kwargs): | |||
|
|
|||
| def register(self, *args, **kwargs) -> None: | |||
| """Register an option.""" | |||
| if args: | |||
There was a problem hiding this comment.
This was needed because we were working with unvalidated args... 🤦
| if ( | ||
| isinstance(type_arg, type) | ||
| and not isinstance(default_value, type_arg) | ||
| and not (issubclass(type_arg, bool) and default_value == UnsetBool) |
There was a problem hiding this comment.
Previously validation tripped up on UnsetBool. I think the only reason this didn't blow up is that we don't actually use UnsetBool anywhere, and the tests were not validating. But now that they are, they caught this.
Possibly we can get rid of UnsetBool, I don't see why None couldn't be used, but one thing at a time.
Previously we validated when parsing, now we do so at registration.
This uncovered a couple of tests that were implicitly relying on
incorrect registration, so fixed those.
Also removed more legacy parser cruft.