-
Notifications
You must be signed in to change notification settings - Fork 993
Check whether less
pager exists before trying to pipe content through it
#6020
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
base: main
Are you sure you want to change the base?
Conversation
Noting that, due to Thanksgiving, I will respond to comments on this PR in December. |
Thank you for the pull request. I don't think it works to change the default pager since it would be a downgrade and create new issues for the majority of users:
So replacing The standard I think if you wanted to attempt to detect |
less
exists on non-Windows systemsless
pager exists before trying to pipe content through it
@mrsdizzie I've updated this to use the same Unfortunately, I don't have a Windows machine handy to test this change. |
Problem
in the corner case where:
wp-cli
is running on a non-Windows systemPAGER
environment variable is not setless
is not installedwp help
command, such as by running that command explicitly or by running an invalidwp-cli
commandwp-cli crashes, as documented in #5333
An example of an environment with out
less
is many "minimalist" Docker server containers.Changes
less
exists in non-Windows environments when the environment variable$PAGER
is not set. Instead, in all environments, invocations ofwp help
will first check to see ifless
exists, and use that, before falling back tomore
. The previous behavior was to pipe output throughless -R
on non-Windows systems without first checking ifless
exists.Help_Command::pass_through_pager()
Alternatives considered
Replacing
less -R
withcat
This would replace a pager with a non-pager, which seems like a feature regression
Replacing
less -R
withmore
in all casesRuled out by @mrsdizzie at #6020 (comment), and removed in b88c438
Accurately checking whether
less
existsI started a branch with this intent, at benlk@832bb1e , but this approach proved nonviable.
To accurately test whether
less
exists:cmd.exe
or Powershell, ascmd.exe
can usewhere
to determine whether a command exists, but Powershell needs to use a different commandcommand -v
accurately determines whether a command exists, we would also need to check for the existence of analias
, which is a separate can of wormsproc_close()
isn't guaranteed to return the command's exit codeThis seemed like way too much scope creep, introducing too much code, requiring too much testing, for a fallback scenario around displaying a help message.
Testing
The existing tests in https://github.com/wp-cli/wp-cli/blob/main/features/help.feature do not care about whether
more
orless
is used.If additional tests are needed for this change, I'm open to suggestions on what should be tested.
For local testing, on all OS:
echo $PAGER
to check what your local shell'sPAGER
variable is. If it is set to any value, including an emptystring, rununset PAGER
to unset it.less
to determine whetherless
exists on your system. If it does, runcommand -v less
(POSIX) orwhere less
(Windows) to determine whether it exists as a binary or as an alias.wp
without any other commands or optionswp help
is displayed. Ifless
exists on your system, it should be paged inless
. Ifmore
exists in your system, it should be paged inmore
.