add support for alias-flags (pantsbuild#19034)

Resolves: pantsbuild#18912 --- This is a first take on solving flag-like aliases by slightly relaxing the requirements on the aliases.: * Aliases may now start with `--`. * Validates against scoped arg names in addition to the original scope name checks. --------- Co-authored-by: Stu Hood <[email protected]> Co-authored-by: Andreas Stenius <[email protected]> Co-authored-by: Joshua Cannon <[email protected]>
sww · Jun 24, 2023 · 70eab47 · 70eab47
1 parent 6cbdd07
commit 70eab47
Show file tree

Hide file tree

Showing 6 changed files with 121 additions and 10 deletions.
diff --git a/pants.toml b/pants.toml
@@ -91,9 +91,8 @@ enabled = true
 repo_id = "7775F8D5-FC58-4DBC-9302-D00AE4A1505F"
 
 [cli.alias]
-all-changed = "--changed-since=HEAD --changed-dependents=transitive"
 run-pyupgrade = "--backend-packages=pants.backend.experimental.python.lint.pyupgrade fmt"
-
+--all-changed = "--changed-since=HEAD --changed-dependents=transitive"
 
 [source]
 root_patterns = [

diff --git a/src/python/pants/option/alias.py b/src/python/pants/option/alias.py
@@ -64,15 +64,15 @@ class CliAlias:
     definitions: FrozenDict[str, tuple[str, ...]] = field(default_factory=FrozenDict)
 
     def __post_init__(self):
-        valid_alias_re = re.compile(r"\w(\w|-)*\w$", re.IGNORECASE)
+        valid_alias_re = re.compile(r"(--)?\w(\w|-)*\w$", re.IGNORECASE)
         for alias in self.definitions.keys():
             if not re.match(valid_alias_re, alias):
                 raise CliAliasInvalidError(
                     softwrap(
                         f"""
                         Invalid alias in `[cli].alias` option: {alias!r}. May only contain alpha
-                        numerical letters and the separators `-` and `_`, and may not begin/end
-                        with a `-`.
+                        numerical letters and the separators `-` and `_`. Flags can be defined using
+                        `--`. A single dash is not allowed.
                         """
                     )
                 )
@@ -104,9 +104,12 @@ def expand(
             )
         )
 
-    def check_name_conflicts(self, known_scopes: dict[str, ScopeInfo]) -> None:
+    def check_name_conflicts(
+        self, known_scopes: dict[str, ScopeInfo], known_flags: dict[str, frozenset[str]]
+    ) -> None:
         for alias in self.definitions.keys():
             scope = known_scopes.get(alias)
+
             if scope:
                 raise CliAliasInvalidError(
                     softwrap(
@@ -117,6 +120,19 @@ def check_name_conflicts(self, known_scopes: dict[str, ScopeInfo]) -> None:
                     )
                 )
 
+        for scope_name, args in known_flags.items():
+            for alias in self.definitions.keys():
+                if alias in args:
+                    scope_name = scope_name or "global"
+                    raise CliAliasInvalidError(
+                        softwrap(
+                            f"""
+                            Invalid flag-like alias in `[cli].alias` option: {alias!r}. This is
+                            already a registered flag in the {scope_name!r} scope.
+                            """
+                        )
+                    )
+
     def expand_args(self, args: tuple[str, ...]) -> tuple[str, ...]:
         if not self.definitions:
             return args

diff --git a/src/python/pants/option/alias_test.py b/src/python/pants/option/alias_test.py
@@ -36,6 +36,13 @@ def test_maybe_expand_alias(alias: str, expanded: tuple[str, ...] | None) -> Non
     )
     assert cli_alias.maybe_expand("alias") == expanded
 
+    cli_alias = CliAlias.from_dict(
+        {
+            "--alias": alias,
+        }
+    )
+    assert cli_alias.maybe_expand("--alias") == expanded
+
 
 @pytest.mark.parametrize(
     "args, expanded",
@@ -60,6 +67,29 @@ def test_expand_args(args: tuple[str, ...], expanded: tuple[str, ...]) -> None:
     assert cli_alias.expand_args(args) == expanded
 
 
+@pytest.mark.parametrize(
+    "args, expanded",
+    [
+        (
+            ("some", "--alias", "target"),
+            ("some", "--flag", "goal", "target"),
+        ),
+        (
+            # Don't touch pass through args.
+            ("some", "--", "--alias", "target"),
+            ("some", "--", "--alias", "target"),
+        ),
+    ],
+)
+def test_expand_args_flag(args: tuple[str, ...], expanded: tuple[str, ...]) -> None:
+    cli_alias = CliAlias.from_dict(
+        {
+            "--alias": "--flag goal",
+        }
+    )
+    assert cli_alias.expand_args(args) == expanded
+
+
 def test_no_expand_when_no_aliases() -> None:
     args = ("./pants",)
     cli_alias = CliAlias()
@@ -110,6 +140,32 @@ def test_no_expand_when_no_aliases() -> None:
                 ),
             ),
         ),
+        (
+            {
+                "cycle": "--other-alias",
+                "--other-alias": "cycle",
+            },
+            pytest.raises(
+                CliAliasCycleError,
+                match=(
+                    r"CLI alias cycle detected in `\[cli\]\.alias` option:\n"
+                    + r"--other-alias -> cycle -> --other-alias"
+                ),
+            ),
+        ),
+        (
+            {
+                "--cycle": "--other-alias",
+                "--other-alias": "--cycle",
+            },
+            pytest.raises(
+                CliAliasCycleError,
+                match=(
+                    r"CLI alias cycle detected in `\[cli\]\.alias` option:\n"
+                    + r"--other-alias -> --cycle -> --other-alias"
+                ),
+            ),
+        ),
     ],
 )
 def test_nested_alias(alias, definitions: dict | ContextManager) -> None:
@@ -128,9 +184,7 @@ def test_nested_alias(alias, definitions: dict | ContextManager) -> None:
         "file.name",
         "target:name",
         "-o",
-        "--o",
         "-option",
-        "--option",
     ],
 )
 def test_invalid_alias_name(alias: str) -> None:
@@ -148,4 +202,35 @@ def test_banned_alias_names() -> None:
             r"Invalid alias in `\[cli\]\.alias` option: 'fmt'\. This is already a registered goal\."
         ),
     ):
-        cli_alias.check_name_conflicts({"fmt": ScopeInfo("fmt", is_goal=True)})
+        cli_alias.check_name_conflicts({"fmt": ScopeInfo("fmt", is_goal=True)}, {})
+
+
+@pytest.mark.parametrize(
+    "alias, info, expected",
+    [
+        (
+            {"--keep-sandboxes": "--foobar"},
+            {"": "--keep-sandboxes"},
+            pytest.raises(
+                CliAliasInvalidError,
+                match=(
+                    r"Invalid flag-like alias in `\[cli\]\.alias` option: '--keep-sandboxes'\. This is already a registered flag in the 'global' scope\."
+                ),
+            ),
+        ),
+        (
+            {"--changed-since": "--foobar"},
+            {"changed": "--changed-since"},
+            pytest.raises(
+                CliAliasInvalidError,
+                match=(
+                    r"Invalid flag-like alias in `\[cli\]\.alias` option: '--changed-since'\. This is already a registered flag in the 'changed' scope\."
+                ),
+            ),
+        ),
+    ],
+)
+def test_banned_alias_flag_names(alias, info, expected) -> None:
+    cli_alias = CliAlias.from_dict(alias)
+    with expected:
+        cli_alias.check_name_conflicts({}, info)
diff --git a/src/python/pants/option/options.py b/src/python/pants/option/options.py
@@ -226,6 +226,10 @@ def unknown_goals(self) -> list[str]:
     def known_scope_to_info(self) -> dict[str, ScopeInfo]:
         return self._known_scope_to_info
 
+    @property
+    def known_scope_to_scoped_args(self) -> dict[str, frozenset[str]]:
+        return {scope: parser.known_scoped_args for scope, parser in self._parser_by_scope.items()}
+
     @property
     def scope_to_flags(self) -> dict[str, list[str]]:
         return self._scope_to_flags

diff --git a/src/python/pants/option/options_bootstrapper.py b/src/python/pants/option/options_bootstrapper.py
@@ -305,7 +305,9 @@ def full_options(
             allow_unknown_options=build_configuration.allow_unknown_options,
         )
         GlobalOptions.validate_instance(options.for_global_scope())
-        self.alias.check_name_conflicts(options.known_scope_to_info)
+        self.alias.check_name_conflicts(
+            options.known_scope_to_info, options.known_scope_to_scoped_args
+        )
         return options
 
 

diff --git a/src/python/pants/option/parser.py b/src/python/pants/option/parser.py
@@ -138,6 +138,11 @@ def scope_info(self) -> ScopeInfo:
     def scope(self) -> str:
         return self._scope
 
+    @property
+    def known_scoped_args(self) -> frozenset[str]:
+        prefix = f"{self.scope}-" if self.scope != GLOBAL_SCOPE else ""
+        return frozenset(f"--{prefix}{arg.lstrip('--')}" for arg in self._known_args)
+
     def history(self, dest: str) -> OptionValueHistory | None:
         return self._history.get(dest)