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 alias for --paging=never #1082

Merged
merged 6 commits into from
Jul 2, 2020
Merged

Add alias for --paging=never #1082

merged 6 commits into from
Jul 2, 2020

Conversation

LordFlashmeow
Copy link
Contributor

@LordFlashmeow LordFlashmeow commented Jun 30, 2020

From #1075, here's my proposed implementation for adding an alias for --paging=never.

I couldn't find a way for clap to create an alias with a value, so I ended up creating a new parameter for it.

I'm not sure if it needs to be added, but I figure I'll share it anyways. I'm not sold on using -P as the flag, but it made the most sense to me.

@eth-p
Copy link
Collaborator

eth-p commented Jul 2, 2020

I think the long flag would be better as --no-paging with an alias of --no-pager. From my experiences, that naming convention is more common than --disable-x.

@sharkdp, thoughts?

@LordFlashmeow
Copy link
Contributor Author

I agree on the naming. Currently there is no long flag, but I'm happy to add that

@sharkdp
Copy link
Owner

sharkdp commented Jul 2, 2020

Thank you very much for your contribution. Code looks good - thank you for adding the tests.

As for the long option, I'm not sure if we really need that. We already have --paging=never as an expressive form (for the config file). I would maybe even go as far as removing the new -P option from the help text (as a separate entry) and modifying the help text of --paging to mention the alias for --paging=never. What do you think?

Could you please add an entry to the "unreleased" section in the CHANGELOG.md? Under "Features". The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or this PR and user is your username.

The man page should also be updated.

@LordFlashmeow
Copy link
Contributor Author

LordFlashmeow commented Jul 2, 2020

Hmm, fair point on the long option, although the --plain option had set a precedent for the flag having a long value and distinct help entry. If you want to move away from that, that's fine too.

Also, should I use the issue or the PR number? Nevermind, just looked at the changelog.

@sharkdp
Copy link
Owner

sharkdp commented Jul 2, 2020

Hmm, fair point on the long option, although the --plain option had set a precedent for the flag having a long value and distinct help entry. If you want to move away from that, that's fine too.

You are right. I think I would still rather keep the --help text shorter by not introducing a completely new line for the option introduced here.

A long alias doesn't make sense, since `--paging=never` already exists
@LordFlashmeow LordFlashmeow marked this pull request as ready for review July 2, 2020 07:07
@LordFlashmeow
Copy link
Contributor Author

I've pulled the long argument and hidden it so it doesn't show up in the help page. I also made a few changes to the pager and paging descriptions in the man page, but I can change it back if I made anything unclear.

Also, as a topic for another issue: adding an environment section to the man page

@sharkdp
Copy link
Owner

sharkdp commented Jul 2, 2020

Thank you very much for the updates and the high quality PR!

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.

3 participants