8000 Add option to hide help based on long/short by stevepentland · Pull Request #1235 · clap-rs/clap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add option to hide help based on long/short #1235

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 2, 2018
Merged

Add option to hide help based on long/short #1235

merged 4 commits into from
Apr 2, 2018

Conversation

stevepentland
Copy link
@stevepentland stevepentland commented Mar 28, 2018 •
edited by kbknapp
Loading

Relates to #1064

I've added the enum variants and related setting methods to provide the functionality. However, the usage of Parser::use_long_help() is causing issues.

It seems that the linked method will not allow use_long to be true unless there is actually a long help value defined for the Arg. However, I noticed in help.rs that we would choose to use the desired long or short help based on what was specified during the run and the extra logic does not seem like it is really needed. However when I bypassed the code in parser.rs, there were some failing tests.

@kbknapp do you have any suggestions?

This change is Reviewable

@stevepentland
Initial addition of new ArgSetting and builder functions
f4dba61
@stevepentland
Add logic to filter based on hidden long/short.
e3dcc01 
There is still an issue with the logic in parser.rs use_long_help. This
causes invalid evaluation of whether to show/hide based on long or short help
@mention-bot
Copy link

@stevepentland, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbknapp to be a potential reviewer.

@kbknapp
Copy link
Member
kbknapp commented Mar 29, 2018

First of all, thanks for tackling this! It looks really good so far!

As for the issue, I'm not sure I understand. You're saying if someone uses Arg::hidden_short_help and hasn't defined any args that have a long_help, then it fails? I see what you mean about Parser::use_long_help making sure an at least one arg has a long_help defined otherwise returning false (which actually sounds wrong to me and should probably be changed...in fact I'm not sure why exactly that is there as --help or -h should be the only determining factor as to which version of the help is displayed...I may dig into this further to remove any unnecessary code), but I'm not seeing where that is affecting your code? It looks correct that you're short circuiting with if Hidden is set, return false from should_show regardless of what use_long says.

If you add some tests I might be able to see the issue a little better.

@stevepentland
Copy link
Author

Sorry, I should have described the issue better, that's my bad. At this point hidden_short_help works 100% when -h is provided.

However, when trying to use hidden_long_help but not actually setting Arg::long_help to anything (which seems reasonable, it is supposed to be hidden on long), the arg is still being shown when --help is given. This is due to Parser::use_long_help always returning false if no long_help has been set, regardless of whether --help was specified or not.

As for extra tests, I will add more to hidden_args.rs in tests as it seems like the most reasonable place.

@kbknapp
Copy link
Member
kbknapp commented Mar 29, 2018

Ah ok I see now. I'm investigating a little more on my end to see what the issue may be, I'll let you know what I find.

@kbknapp
Copy link
Member
kbknapp commented Mar 29, 2018

Ok, so I see what's going on now (at least the Parser::use_long_help). It was meant to be able to say, "If no args defined a long_help just go ahead and use the old short help style." So people's help messages didn't suddenly change.

i.e.

Old style when using --help and no long_help has been defined

test 1.3
Kevin K.
tests stuff

USAGE:
    test [OPTIONS] [arg1]

OPTIONS:
    -f, --flag            some flag
    -h, --help            Prints help information
        --option <opt>    some option
    -V, --version         Prints version information

ARGS:
    <arg1>    some pos arg

Without Parser::use_long_help, even without defining any long_help, when the user does --help the message changes to this:

test 1.3
Kevin K.
tests stuff

USAGE:
    test [OPTIONS] [arg1]

OPTIONS:
    -f, --flag            
            some flag

    -h, --help            
            Prints help information

        --option <opt>    
            some option

    -V, --version         
            Prints version information


ARGS:
    <arg1>    
            some pos arg

(note the help text being printed on the line below).

I'm personally fine with this change and dropping Parser::use_long_help entirely and if the user uses --help they see the long message, otherwise they see the short message with -h. I've never considered the format of the output of the help messages to part of the public API, only the correctness. It's had small formatting changes and such throughout v1 and v2.

However, it is a somewhat in your face stylistic change and would like to hear a few other opinions before making that particular change.

Back to the issue at hand, a simple way to keep the old functionality and fix this issue is to not only check if long_help.is_some() on all args, but also .is_set(ArgSettings::HiddenLongHelp).

@stevepentland
Copy link
Author

Back to the issue at hand, a simple way to keep the old functionality and fix this issue is to not only check if long_help.is_some() on all args, but also .is_set(ArgSettings::HiddenLongHelp).

It does work, but it also brings up another potential issue: While there is no change when hidden_long_help has not been set, once it has been, the output suddenly changes to the style you outlined in your comment. This may catch some people off guard if they're adding hidden_long_help and have no previously defined long_help values as now there are 2 different styles for --help.

@kbknapp
Copy link
Member
kbknapp commented Mar 30, 2018

So long as it's documented I'm fine with it. I think the case to hide something from the long help only will be extremely rare, and already somewhat implies two different help messages (even if style isn't implied).

@kbknapp
Copy link
Member
kbknapp commented Apr 1, 2018

@stevepentland let me know once you're good with it and I'll do the final review

@stevepentland
Copy link
Author

@kbknapp will do, should be soon. Just gotten side tracked

@stevepentland
Copy link
Author

@kbknapp I've gotten them both working, updated the check, and added docs stating that using these options will change how --help displays. I also added tests.

@kbknapp
Copy link
Member
kbknapp commented Apr 2, 2018

Looks good, thanks! 👍

@kbknapp kbknapp merged commit d51d35d into clap-rs:master Apr 2, 2018
This was referenced Apr 2, 2018
Closed
Closed
@stevepentland stevepentland deleted the hide-help branch April 3, 2018 19:50
@kbknapp kbknapp mentioned this pull request Apr 4, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stevepentland @mention-bot @kbknapp
0