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.

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

fix: hide subcommand from short help when no subcommands #202

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: hide subcommand from short help when no subcommands #202

wants to merge 2 commits into from

Conversation

olizilla
Copy link
Member

@olizilla olizilla commented Oct 22, 2020 •
edited
Loading

ShortHelp always shows the subcommand help prompt even if there are no subcommands.

$ ipfs version
ipfs version 0.7.0

$ ipfs ls -h
USAGE
  ipfs ls <ipfs-path>... - List directory contents for Unix filesystem objects.

  ipfs ls [--headers | -v] [--resolve-type=false] [--size=false] [--stream | -s] [--] <ipfs-path>...

  Displays the contents of an IPFS or IPNS object(s) at the given path, with
  the following format:
  
    <link base58 hash> <link size in bytes> <link name>
  
  The JSON output contains type information.

  For more information about each command, use:
  'ipfs ls <subcmd> --help'

The last 2 lines of the short help are supposed to let the user know that --help will get them more help about this command. There should be no invitation to check subcommands if there are no subcommands for this command.

This PR changes things so we:

  • only invite the user to check help for subcommands if there are any
  • invite the user to check out the LongHelp for this command with --help

License: MIT
Signed-off-by: Oli Evans [email protected]

@olizilla
wip: show subcommand ShortHelp bug with test
b0de627 
ShortHelp always shows the subcommand help prompt even if there are no subcommands.

Add a test to demo it.

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@welcome
Copy link

welcome bot commented Oct 22, 2020

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions
    and stakeholders linked/mentioned.

  • Your contribution itself is clear (code comments, self-review for the
    rest) and in its best form. Follow the code contribution
    guidelines
    if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on
    any missing things and potentially assigning a reviewer for high
    priority items.

  • The PR gets reviews, discussed and approvals as needed.

  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@olizilla
fix: show subcommand info in short help if there are any
8025471 
- only invite the user to check help for subcommands if there are any
- invite the user to check out the LongHelp for this command with --help

License: MIT
Signed-off-by: Oli Evans <[email protected]>
@olizilla olizilla force-pushed the hide-subcommand-help branch from cf7afb2 to 8025471 Compare October 22, 2020 19:45
@olizilla olizilla requested a review from aschmahmann October 23, 2020 08:32
aschmahmann
aschmahmann reviewed Nov 10, 2020
View reviewed changes
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Left a few tiny suggestions/comments but LGTM. Thanks 😄.

Note: Does this effect our documentation generation at all or does it not utilize this codepath?

cli/helptext.go
Comment on lines 113 to 114
{{.Indent}}For more information about each command, use:
{{.Indent}}'{{.Path}} <subcmd> --help'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it at all confusing that we let people know explicitly that they can run --help, but not the short -h?

I suspect not but figured I'd check in.

cli/helptext_test.go
@@ -48,3 +49,47 @@ func TestSynopsisGenerator(t *testing.T) {
t.Fatal("Synopsis should contain options finalizer")
}
}

func TestShortHelp(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add a test that checks on subcommands? The two things I'd like to watch for are:

  1. I'd like some signal that these tests are still useful even if someone changes "For more information about each command" to be rephrased slightly. This could also be aided by checking a constant.
  2. Making sure that subcommands are printed

cli/helptext.go
{{.Indent}}For more information about each command, use:
{{.Indent}}'{{.Path}} <subcmd> --help'
{{end}}{{if .MoreHelp}}
{{.Indent}}for more information about this command, use:
Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase f here intentional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschmahmann aschmahmann aschmahmann left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@olizilla @aschmahmann