-
Notifications
You must be signed in to change notification settings - Fork 605
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 --no-pager
option in help
command
#5007
Conversation
738c53c
to
181ddaf
Compare
crates/uv/src/commands/help.rs
Outdated
@@ -67,11 +67,11 @@ pub(crate) fn help(query: &[String], printer: Printer) -> Result<ExitStatus> { | |||
}; | |||
|
|||
let is_terminal = std::io::stdout().is_terminal(); | |||
if !is_root && is_terminal && which("less").is_ok() { | |||
if !no_pager && !is_root && is_terminal && which("less").is_ok() { |
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.
Maybe we should collapse these into a should_page
bool haha it's getting kind of long.
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.
Will do! 👍
Hm yeah this is tough. We may need to manually write out the help template so it includes the options for the command? Unless there's a way to turn off the global options (I don't think there is) |
fn test_with_no_pager() { | ||
let context = TestContext::new_with_versions(&[]); | ||
|
||
uv_snapshot!(context.filters(), context.help().arg("--no-pager"), @r###" |
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.
We never page for the root command so --no-pager
should have no effect here (we also can't page during snapshots so it's kind of moot).
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.
Should I remove the test then?
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.
Seems nice to confirm that the option exists and works. Maybe just add a comment noting that we can't actually check if the pager is used?
Thanks for contributing! Congrats on the first pull request :) |
Looking at the template documentation doesn't seem like there's a way to separate them. 😕 |
cf94cd1
to
bee875e
Compare
Fixed all the things you pointed out. I also rebased to bring the changes from #5005 or the new test would have failed right after merging in |
Summary
Fixes #4941.
This PR adds a
--no-pager
option inhelp
command to explicitly disable the pager.I noted that the template used for the text printed when calling
help
with no argument or option doesn't show any option. It made sense before this PR sincehelp
didn't have any available option. Though I'm unsure if it makes sense to update the template as it would make it extremely verbose as all the global options would be shown too.I leave the decision to you.
Test Plan
I ran
cargo run -- help
to verify--isolated
was visible and it.I ran clippy with
cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
as CI does.I also ran tests locally with: