-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
Options parsing shouldn't care that positional args are specs. #8602
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This mostly just renames the `target_specs` fields in ArgSplitter and Options to `positional_args`. Consumers of parsed command lines can know that the positional args represent target specs, but there's no need for the cmd line parser itself to know this. Note that in practice the arg splitter still ~knows that positional args "look like" target specs or paths, so that it can tell them apart from unknown goals. However we can expand the positional arg detection heuristic in the future, to include things that aren't paths. This change breaks out that heuristic into a method, so that we can modify it in the future. This change also removes the old affordance for inserting the superfluous word `goal` in command lines. E.g., `./pants goal compile ...` instead of `./pants compile ...`. That was a legacy of early versions of pants, and has not been "a thing" for a very long time.
Eric-Arellano
approved these changes
Nov 9, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thank you for breaking this out into a dedicated PR.
passthru: Any remaining args specified after a -- separator. | ||
passthru_owner: The scope specified last on the command line, if any. None otherwise. | ||
""" | ||
@dataclass(frozen=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for typing this!
stuhood
approved these changes
Nov 9, 2019
It causes knock-on effects that stymie mypy, and I don't want to go down that rabbit hole in this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This mostly just renames the
target_specs
fields in ArgSplitterand Options to
positional_args
. Consumers of parsed commandlines can know that the positional args represent target specs,
but there's no need for the cmd line parser itself to know this.
Note that in practice the arg splitter still ~knows that positional
args "look like" target specs or paths, so that it can tell them
apart from unknown goals. However we can expand the positional
arg detection heuristic in the future, to include things that
aren't paths. This change breaks out that heuristic into a method,
so that we can modify it in the future.
This change also removes the old affordance for inserting the
superfluous word
goal
in command lines. E.g.,./pants goal compile ...
instead of./pants compile ...
.That was a legacy of early versions of pants, and has not
been "a thing" for a very long time.