-
Notifications
You must be signed in to change notification settings - Fork 348
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
Use minus
as a builtin pager
#3024
Conversation
8e0b39b
to
7d77274
Compare
This is currently using a branch of mine so that I could see if I could keep the |
14d1022
to
d963300
Compare
d963300
to
ee56291
Compare
One downside is that The |
ee56291
to
69df874
Compare
I've been using |
841e468
to
e71d2da
Compare
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.
LGTM; I'll give this a spin on my Windows box later tonight if nobody else gets to it in the meantime, but I'm excited to see this hole plugged up a bit.
As a side note, I'd personally prefer if we tried to avoid using forks via cargo git dependencies (it's just so easy to let them sit around for too long, in my experience, and it makes the Nix build more annoying and fiddlier); so just a follow up patch would be best, IMO. Or just hold off for a day or so if it'll get merged soon anyway.
0e7a475
to
9ada916
Compare
Very supportive of not pinning git repos 👍. I think if for some reason (which I don't think will happen) we can't work with Also, after actively using it for a couple hours I think it's actually better without my patch until I figure out how to do early exit with Do you wanna give it a try on Windows first or should we merge as is? |
3482afd
to
c3fd9ed
Compare
Hey guys! So I hope I am not being an unwanted guest here. @benbrittain actually mentioned about this in a PR. So I came here to take a look at the actual code being written. While reading your comments, I found this.
I don't think it's any true as of today. I had written that comment for <=v4.x.x releases which used a completely different model for text data that didn't allow this functionality for dynamic mode. As for v5.x.x releases this should be possible. I didn't implement this because it never crossed my mind earlier. So if you guys want this, just throw out a issue atleast. |
@arijit79 you are more than welcome here! Thank you for writing |
c3fd9ed
to
1837465
Compare
Added a built-in pager since the solution in #2928 felt hacky. Also should address #2044
Checklist
If applicable:
CHANGELOG.md