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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make --no-paging/-P override --paging=... if passed as a later arg #2201

Merged
merged 2 commits into from
Aug 14, 2022

Conversation

themkat
Copy link
Contributor

@themkat themkat commented May 23, 2022

Part of fix for #2128

Makes the -P and -pp option take precedence over --paging when given. See issue for more details 馃檪 Had to change one test, as it assumed the opposite of what the issue is explaining.

@themkat themkat changed the title Disable paging when -P flag or plain mode -pp is active Always disable paging when -P flag or plain mode -pp is active May 23, 2022
@themkat themkat changed the title Always disable paging when -P flag or plain mode -pp is active Disable paging if -P is a later argument May 24, 2022
@themkat themkat requested a review from sharkdp May 24, 2022 17:55
@themkat themkat closed this Jul 18, 2022
@dufferzafar
Copy link

@themkat Why was the PR closed? Due to pending review? Or does the PR not actually solve the issue?

@themkat
Copy link
Contributor Author

themkat commented Aug 5, 2022

@dufferzafar It solves the issue with -P as a later argument, but not the double -pp, as I found no way of solving that. Also, I interpreted a second review never happening as a sign that the maintainers didn't want it. So I closed it for that reason alone. I was very new to Rust at the time, and was just playing around. Probably also thinking to myself that it was not good enough 馃し

@dufferzafar
Copy link

Yeah, I myself can't think of how -pp override would work with clap, but I think having -P is good as well. You should consider re-opening the PR and then asking a reviewer to have a look at it again.

@themkat themkat reopened this Aug 5, 2022
@themkat
Copy link
Contributor Author

themkat commented Aug 5, 2022

@dufferzafar Thanks for the interest in it 馃槃 I have now reopened the PR.

@Enselic Enselic changed the title Disable paging if -P is a later argument Make --no-paging/-P override --paging=... if passed as a later arg Aug 14, 2022
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! Sorry for letting you wait 馃槄

I have confirmed the test fails without the fix, and I think this is a completely reasonable little improvement, so I rebased it, updated CHANGELOG, and will merge after CI is green.

Thanks again!

@Enselic Enselic merged commit 9c7ca33 into sharkdp:master Aug 14, 2022
@themkat themkat deleted the GH-2128 branch August 14, 2022 19:13
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.

None yet

4 participants