Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rip out more parts of the legacy parser #21643

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Nov 13, 2024

No description provided.

@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Nov 13, 2024
@benjyw benjyw requested review from tdyas and huonw November 13, 2024 23:59
from pants.util.frozendict import LazyFrozenDict
from pants.util.strutil import first_paragraph, strval

T = TypeVar("T")

_ENV_SANITIZER_RE = re.compile(r"[.-]")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is now the only place using this, so it moved here from parser.py

# Global options have three env var variants. The last one is the most human-friendly.
env_var = Parser.get_env_var_names(self._scope, dest)[-1]
udest = dest.upper()
if self._scope == GLOBAL_SCOPE:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is (simplified) logic moved from parser.py.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my information: we take only the last element, so we can discard the logic that computes the other values

@@ -107,24 +92,14 @@ def _invert(cls, s: bool | str | None) -> bool | None:
b = cls.ensure_bool(s)
return not b

@classmethod
def scope_str(cls, scope: str) -> str:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was only used in Parser._scope_str() below, so is now inlined there.

env_vars = [f"PANTS_{sanitized_env_var_scope}_{udest}"]
return env_vars

def _compute_value(self, dest, kwargs, flag_val_strs, passthru_arg_strs, log_warnings):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting this is the big win in this PR.

Copy link
Contributor

@lilatomic lilatomic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there tests that we could remove since all of this is now on the Rust side? Or do we want to keep them now as integration tests

@@ -1106,9 +1109,16 @@ def get_option_help_info(self, args, kwargs):
removal_hint = kwargs.get("removal_hint")
choices = self.compute_choices(kwargs)

dest = Parser.parse_dest(*args, **kwargs)
dest = parse_dest(*args, **kwargs)
# Global options have three env var variants. The last one is the most human-friendly.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Global options have three env var variants. The last one is the most human-friendly.
# Global options have three env var variants
# (eg. PANTS_GLOBAL_PANTS_WORKDIR, PANTS_PANTS_WORKDIR, PANTS_WORKDIR).
# The last one is the most human-friendly.

maybe port these from the comment since its being deleted

# Global options have three env var variants. The last one is the most human-friendly.
env_var = Parser.get_env_var_names(self._scope, dest)[-1]
udest = dest.upper()
if self._scope == GLOBAL_SCOPE:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my information: we take only the last element, so we can discard the logic that computes the other values

@benjyw
Copy link
Contributor Author

benjyw commented Nov 14, 2024

Are there tests that we could remove since all of this is now on the Rust side? Or do we want to keep them now as integration tests

Not yet, no. There will be some soon though.

@benjyw benjyw merged commit c24b407 into pantsbuild:main Nov 14, 2024
24 checks passed
@benjyw benjyw deleted the rip_out_legacy_parser2 branch November 14, 2024 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants