-
Notifications
You must be signed in to change notification settings - Fork 41
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 an output pager. #96
Conversation
|
||
|
||
def resolve_output_path(output_path): | ||
@contextmanager | ||
def resolve_output_path(output_path, allow_pager=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.
We should probably support core.pager
from git config, maybe =sno
and =false
? I turn it off in CI E2E tests since the CI envs have TTY things so output is pretty.
In retrospect, this is quite roundabout. If I implement something in click itself I can just yield the filelike object it's already using for stdin to less. i.e. bypass the generator interface altogether. Then I dont need threads :) I'll work on a click PR |
Looks reasonable. I'm not sure if Click currently supports it, but does your approach break combining a pager with terminal output because the stream is closed when the context manager exits? eg: prompt for X, view some text in the pager, quit the pager, prompt for Z, view some more output, ... |
687397c
to
4ad8654
Compare
maybe one day we'll return to this, but it's been almost three years, I'm sure we'll do it differently anyway |
Fixes #50
What a mission. This took way longer than I expected.
But I'm fairly happy with the results:
Notes:
I changedresolve_output_path
to a contextmanager, and made it yield a file-like object that pushes lines of text into a queue, which are read by a worker thread which yields them viaclick.echo_via_pager
.I implemented pallets/click#1572 to support doing this without having to use threads... that should ideally be merged and released before this PR is merged.