Port the cli aliasing feature to rust #21695
Conversation
| @@ -692,6 +692,7 @@ class PyOptionParser: | |||
| self, option_id: PyOptionId, default: list[str] | |||
| ) -> OptionValue[list[str]]: ... | |||
| def get_dict(self, option_id: PyOptionId, default: dict[str, Any]) -> OptionValue[dict]: ... | |||
| def get_args(self) -> list[str]: ... | |||
There was a problem hiding this comment.
We must now get the expanded args back from the native engine to pass to ArgSplitter.
| @@ -141,8 +141,18 @@ def create( | |||
| # We need registrars for all the intermediate scopes, so inherited option values | |||
| # can propagate through them. | |||
| complete_known_scope_infos = cls.complete_scopes(known_scope_infos) | |||
|
|
|||
| config_to_pass = None if native_options_config_discovery else config.sources() | |||
There was a problem hiding this comment.
This is just moved up from below, so we can create the native_parser earlier and use it to get the args to split.
| @@ -121,6 +119,7 @@ def create( | |||
| absolute paths. Production use-cases should pass True to allow options values to make the | |||
| decision of whether to respect pantsrc files. | |||
| """ | |||
| args = tuple(args) | |||
There was a problem hiding this comment.
To ensure we can fingerprint them (previously we inadvertently relied on CliAlias tupling them for us.
| @@ -407,70 +407,6 @@ def test_setting_pants_config_in_config(self, tmp_path: Path) -> None: | |||
| logdir = ob.get_bootstrap_options().for_global_scope().logdir | |||
| assert "logdir1" == logdir | |||
|
|
|||
| def test_alias(self, tmp_path: Path) -> None: | |||
There was a problem hiding this comment.
This is now a test on Options, instead of on OptionsBootstrapper, so it has moved (suitably modified) to options_test.py
| @@ -1142,6 +1142,64 @@ def test_passthru_args_not_interpreted(): | |||
| ).string | |||
|
|
|||
|
|
|||
| def test_alias() -> None: | |||
There was a problem hiding this comment.
There is ample testing of the aliasing itself on the rust side, but this is a good test that it all fits together correctly in the bootstrapping.
| @@ -18,7 +18,8 @@ whoami = { workspace = true } | |||
| serde = { workspace = true, features = ["derive"] } | |||
| serde_json = { workspace = true } | |||
| serde_yaml = { workspace = true } | |||
| parking_lot = "0.12.3" | |||
| parking_lot = { workspace = true } | |||
There was a problem hiding this comment.
This was an oversight, it should always have been taken from the workspace version.
benjyw
force-pushed
the
port_cli_aliasing_to_rust
branch
from
7b6ebab to
2fbc12e
Compare
| // Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). | ||
| // Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
|
||
| use crate::Scope; |
There was a problem hiding this comment.
This was ported directly from the python version, as were its tests
| // Copyright 2024 Pants project contributors (see CONTRIBUTORS.md). | ||
| // Licensed under the Apache License, Version 2.0 (see LICENSE). | ||
|
|
||
| use crate::cli_alias; |
There was a problem hiding this comment.
All the tests from the python version were replicated here.
| } | ||
|
|
||
| #[test] | ||
| fn test_deep_alias_cycle() { |
There was a problem hiding this comment.
This was added here and is not in the python tests, just for good measure.
|
(Just acknowledging the review request, I have been too busy for the past few days, should have some time next week.) |
|
@kaos, since CLI Aliasing was your baby, you might want to take a pass at this? |
yea, this is a bit involved, and it's been quite a while and rust is not my strong side so it would be quite an effort on my part to load this all in again. Sorry ;) If there's anything weird or strange I've done you need me to look at on the python side of the old thing I could dig into that. |
|
@huonw All comments addressed. PTAL. It is probably worth looking at just the new commit... It touches a lot of files, but mostly in very small ways. Thanks! |
This is a step on the road to getting rid of the
legacy options parser code, and greatly simplifying
the OptionsBootstrapper.
A followup change will remove the legacy CliAlias code.