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
Add ./pants help subsystems
#11088
Add ./pants help subsystems
#11088
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
With my latest changes when running
|
subsystems
from help commandsubsystems
from help command
@Eric-Arellano and @benjyw this is ready for review 😃 |
Thanks @thamenato ! Will review tomorrow. |
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.
This looks great! Thanks!
As mentioned over DM, would be good to add 1-2 tests.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
Awesome! This looks great.
"pex How Pants uses Pex to run Python subprocesses" in pants_run.stdout | ||
) | ||
assert "to get help for a specific subsystem" in pants_run.stdout | ||
assert "test Run tests." not in pants_run.stdout |
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.
Negative assertions are generally brittle. If we rewrite the docstring for TestSubsystem
to be different than "Run tests.", then this assertion will always pass, even if we break things. https://thepugautomatic.com/2013/04/negative-assertions/ talks a bit about this.
I suspect the best thing here would be to still use a negative assertion, but to use the re
module to make it less brittle. Maybe something like ^test\s+
. It's plausible we'll change the docstring and/or number of spaces, but very unlikely we'd ever change the line starting with test
imo.
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.
Yeah, I was wondering that myself.. it wouldn't fail but putting only the word "test" is also not a good idea. I think what you said is definitely a good solution, using some regex ❤️
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.
but putting only the word "test" is also not a good idea
Indeed, and this assertion wouldn't work either. test
is part of pytest
hehe.
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.
Done, updated it to use regex.
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
Woot! Will merge on green (unless Benjy has more feedback)
Thanks!
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.
LGTM!
This is a great contribution, thanks for doing it! |
subsystems
from help command./pants help subsystems
FYI I reworded the PR title to "Add |
[ci skip-rust]
[ci skip-build-wheels]
Problem
As described on #11032:
Solution
Implement the "category"
subsystems
inside the help function.Result
When running the command
./pants help subsystems
the user should be able to see what are the subsystems availables.