Skip to content
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 file-like pager: click.get_pager_file() #1572

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

craigds
Copy link

@craigds craigds commented Jun 2, 2020

In koordinates/kart we're in need of an output pager, and are already using click.

Trouble is, passing a generator to echo_via_pager requires that you're producing output in one place. That's not the case with a reasonably complex command - functions and modules delegating to each other all over the place.

To use echo_via_pager, I essentially had to use some kind of concurrency - async or threads - and have my code feed output to a worker thread which would yield the output from a generator function, which echo_via_pager would consume.

That worked, but then I realised click could just expose the file-like object it's using internally (which is usually a pipe to less).

This PR does that. click.get_pager_file exposes a writable file like object which feeds into a pager. The semantics of click.echo_via_pager haven't changed, but it now uses click.get_pager_file under the hood and merely feeds the generator contents into the file.

craigds added a commit to koordinates/kart that referenced this pull request Jun 2, 2020
@davidism
Copy link
Member

davidism commented Jun 4, 2020

Thanks for looking at this, it's definitely an interesting approach. Have you confirmed that this works on Windows? Have you tested it with different pagers on Linux?

@craigds
Copy link
Author

craigds commented Jun 4, 2020

Thanks for taking a look.

I've tested both _pipepager and _tempfilepager paths, but only with less and only on MacOS. I'll try with linux builds and with other pagers, and probably I can rope @rcoup into testing on Windows for me...

@craigds
Copy link
Author

craigds commented Jun 5, 2020

Extra testing is still TODO, but the new commits are a bit more reviewable, esp if you read them one at a time 👍

@craigds
Copy link
Author

craigds commented Jun 5, 2020

On MacOS and Linux (I did it with the python:3.8 docker image) output works as expected with PAGER=cat, PAGER=less, PAGER=more.

craigds added a commit to koordinates/kart that referenced this pull request Jun 5, 2020
craigds added a commit to koordinates/kart that referenced this pull request Jun 5, 2020
craigds added a commit to koordinates/kart that referenced this pull request Jun 5, 2020
craigds added a commit to koordinates/kart that referenced this pull request Jun 9, 2020
@stefreak
Copy link
Contributor

stefreak commented Aug 6, 2021

I love this one. It improves the code a lot. What is missing except for a round of conflict resolution? Does it really need extra test or is it covered by test of echo_via_pager?

@craigds
Copy link
Author

craigds commented Aug 7, 2021

It needs testing in windows. The little testing I did suggested that it doesn't really work on windows yet. I got stuck trying to set up any kind of dev environment on windows to improve it. If anyone who has a windows setup already could help, that'd be much appreciated

@craigds
Copy link
Author

craigds commented Aug 7, 2021

the mypy stuff is also blowing my mind a bit to be honest. I just spent about an hour trying to resolve all the type warnings and haven't yet managed to figure out what to do with things like variables that can hold either a text stream or a bytes stream

@stefreak
Copy link
Contributor

stefreak commented Sep 7, 2024

@craigds would you mind if I give this another shot once #2775 has been merged? I rented a Hetzner windows server for development tasks like this and could look into adding tests for windows.

@craigds
Copy link
Author

craigds commented Sep 8, 2024

@stefreak yep that'd be awesome. I'll have a go at updating this PR for you

@craigds
Copy link
Author

craigds commented Sep 8, 2024

@stefreak I've updated this PR to latest main and it works okay for me on MacOS:

Screenshot 2024-09-09 at 11 36 07 AM

@AndreasBackx AndreasBackx mentioned this pull request Oct 26, 2024
39 tasks
@AndreasBackx
Copy link
Collaborator

@stefreak @craigds this might be able make it into 8.2.0. If you rebase this PR on #2775, I'm happy to accept a "stacked PR" like that. It should be possible to get #2775 in 8.2.0, see #2789 for more info. I'm working through all 8.1.8 and 8.2.0 potential PRs right now and updating that issue and a new one as well.

@AndreasBackx AndreasBackx added this to the 8.2.0 milestone Oct 26, 2024
This better exposes the file-like objects yielded by the three private
pager functions, and also slightly reduces some code duplication
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants