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

switch builtin pager to streampager #4203

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

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Aug 3, 2024

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Cargo.toml Outdated Show resolved Hide resolved
@ilyagr
Copy link
Contributor

ilyagr commented Aug 3, 2024

I would really like a version of streampager with its dependencies updated for this (can/should we ask the Facebook people to do it? Should we fork it?). It's annoying to have to link an old version of clap (see the diff in Cargo.toml).

IIRC, streampager's interface seemed a bit annoying in parts by default, so we might want to change its default config (or try it out at least).

Apart from that, I think this might be great!

@yuja
Copy link
Contributor Author

yuja commented Aug 3, 2024

I would really like a version of streampager with its dependencies updated for this

Totally agree, and the stripped-down version exists in Sapling tree. Asked question at markbt/streampager#61

@yuja yuja marked this pull request as draft August 3, 2024 14:30
@yuja yuja force-pushed the push-vwqpwvoxokrt branch from 8cb0ad3 to 193516d Compare August 5, 2024 02:16
@yuja yuja force-pushed the push-vwqpwvoxokrt branch from 193516d to 7ea0283 Compare August 17, 2024 03:25
@yuja yuja force-pushed the push-vwqpwvoxokrt branch 3 times, most recently from 2c07fa0 to b2e8fdf Compare January 12, 2025 08:45
@yuja yuja changed the title RFC: switch builtin pager to streampager switch builtin pager to streampager Jan 12, 2025
@yuja yuja marked this pull request as ready for review January 12, 2025 09:08
@yuja
Copy link
Contributor Author

yuja commented Jan 12, 2025

Updated dependency to sapling-streampager.

yuja added 2 commits January 12, 2025 19:31
This helps work around dependency version conflicts with streampager.
Perhaps, the "windows" feature was enabled through "minus" or "scm-record". If
I removed "minus", the Windows build of crossterm 0.27.0 failed.
@yuja yuja force-pushed the push-vwqpwvoxokrt branch from b2e8fdf to 097b69f Compare January 12, 2025 10:31
yuja added 4 commits January 12, 2025 19:58
The WTFPL license is added to the allow list. I've never heard about this
license, but it's basically the same as public domain according to wikipedia.
According to the discussion on Discord, streampager is still maintained.
It brings more dependencies, but seems more reliable in our use case. For
example, the streampager doesn't consume inputs indefinitely whereas minus
does. We can also use OS-level pipe to redirect child stderr to the pager.
@yuja yuja force-pushed the push-vwqpwvoxokrt branch from 097b69f to 7bf4644 Compare January 12, 2025 10:59
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.

2 participants