-
Notifications
You must be signed in to change notification settings - Fork 125
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
#36 add -p --pager flag to page the output using pager crate #44
Conversation
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.
Oh, the pager crate looks really nice 🙂 Perfect fit for tealdeer, I think.
@dbrgn is there any pending issue about this ? |
No, I just needed to find some free time to review 🙂 My main issue with this is currently that the default pager is In my opinion the We could force Furthermore, it doesn't seem to be supported on Windows: https://gitlab.com/imp/pager-rs/issues/5 So we should probably exclude the pager functionality when building for Windows. I'll test the PR on macOS next week. Regarding defaults, I'm not sure yet whether paging should be the default behavior or not. Probably not, but we can add it to the configuration file once #43 has landed. |
I will keep the -p while #43 is in progress and then we can use the configuration file and implement the use of PAGER env var on our own. I mean, we can follow a priority to check if the output has to be paged:
Once we know we have to page, check if any pager is configure (PAGER env var or config file), otherwise use |
yep, totally agree, let's change by |
I opened an issue: https://gitlab.com/imp/pager-rs/issues/10 |
It seems that the |
Released in pager 0.15! This should do:
Also, in the meantime configfile support was merged 🙂 |
Thank you @dbrgn, I will update my PR today. I will also include an option for the configuration file as we commented previously on this thread. |
Totally forgot about this 🙏. I will try to finish the task. |
@dbrgn I have already tested this on Linux with several commands. I think is ready to be merge |
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.
Some feedback 🙂
Tanks for the feedback @dbrgn I have updated the code. |
Thanks @jdvr! For some reason the PR deletes the |
Without this change, a config without a [display] section would fail to parse.
Huh, what's wrong with Travis... The tests passed but the state wasn't updated. |
I am happy to see this merged after a lot of time 😁 |
Oh no, this broke Windows support :( pager-rs doesn't support Windows (yet): https://gitlab.com/imp/pager-rs/issues/5 |
Oh, too bad 🤕 We need Windows CI, otherwise this might happen again in the future. Maybe we could simply disable pager support on Windows? |
Yeah, that's a good idea. I'll try to take a look next weekend when I should hopefully have some time. Aside from that, pager support on Unix sounds great! |
I just added the pager crate and a flag (
-p --pager
) to enable or disable it.BTW, I have tested it with Rust 1.28.0 and it works well, maybe you can update.
I would need help for windows and macOS testers.
refs #36