Add default values to web manual and man pages

[zsloane]

Fixes #8387

It looks like the help text is largely controlled by Cobra internally, so I tried to make my changes match what Cobra outputs.

I added tests but if it would be better to test each flag default condition separately rather than in two larger tests, I can split them out.

I wasn't sure how exactly each flag type should be handled but my assumptions were:

• Booleans (false defaults) and empty strings/slices should just be hidden
• Though they don't make much sense boolean flags with true defaults are technically possible so they should be shown

Also, this was my first time writing any Go (and my first open source contribution) so hopefully what I came up with isn't too far off beat.

[andyfeller]

@zsloane : thank you for opening up this PR and your patience! 🙇

Couple of minor suggestions but this looks good. I do notice some flags with and without double quotes which I don't know if is intended or not. Thoughts?

[zsloane]

Nice catch. It should be consistent. The cobra help text generally has lowercase default and no colon. The other thing I realized I am missing is quotes for strings (gh issue list --state -> (default open) versus (default "open") in the help text). I will try and figure that out.

I also discovered that some flags have (default: ...) in the usage string rather than having anything in the flag default arg. See below for some examples. This doesn't look like it will affect those since the actual cobra command defaults in these cases are empty strings.

cli/pkg/cmd/release/create/create.go

Line 181 in 6309446

cmd.Flags().StringVar(&opts.Target, "target", "", "Target `branch` or full commit SHA (default: main branch)")

cli/pkg/cmd/gist/create/create.go

Line 99 in 6eb6501

cmd.Flags().BoolVarP(&opts.Public, "public", "p", false, "List the gist publicly (default: secret)")

cli/pkg/cmd/api/api.go

Line 229 in 6309446

cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "The GitHub hostname for the request (default \"github.com\")")

[andyfeller]

Nice catch. It should be consistent.

Man after my own heart ❤️

The cobra help text generally has lowercase default and no colon. The other thing I realized I am missing is quotes for strings (gh issue list --state -> (default open) versus (default "open") in the help text). I will try and figure that out.

Assuming that it is more effort to change how Cobra formats defaults, I assume we do what we can to match it. What are your thoughts?

I also discovered that some flags have (default: ...) in the usage string rather than having anything in the flag default arg. See below for some examples. This doesn't look like it will affect those since the actual cobra command defaults in these cases are empty strings.

cli/pkg/cmd/release/create/create.go

Line 181 in 6309446

cmd.Flags().StringVar(&opts.Target, "target", "", "Target `branch` or full commit SHA (default: main branch)")

cli/pkg/cmd/gist/create/create.go

Line 99 in 6eb6501

cmd.Flags().BoolVarP(&opts.Public, "public", "p", false, "List the gist publicly (default: secret)")

cli/pkg/cmd/api/api.go

Line 229 in 6309446

cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "The GitHub hostname for the request (default \"github.com\")")

Interesting. Without tracing all of these code paths to see why the defaults are not set in the command, I suppose we leave them be?

[zsloane]

Assuming that it is more effort to change how cobra (I think it's technically pflag handling this) formats defaults, I assume we do what we can to match it. What are your thoughts?

I prefer (default: ...) over (default ...) but it should just match what already is shown with the help text.

This got a bit tricksy as I dug in some more. When trying to verify the requested changes I noticed another special case in how cobra handles the display of the defaults.

For example this has a default of 0 and .DefVal -> 0s (which seems like a reasonable default to show) but cobra hides it:

cli/pkg/cmd/api/api.go

Line 241 in cd2a6ad

cmd.Flags().DurationVar(&opts.CacheTTL, "cache", 0, "Cache the response, e.g. \"3600s\", \"60m\", \"1h\"")

At first I thought the "duration" type is handled differently so I added support for hiding based on flag type. However, cobra shows (default 10s) etc just fine so instead I ended up special casing the .DefVal -> 0s like the others. If for some reason in the future a particular type needs to be hidden it can just be added to hiddenFlagDefaults

Interesting. Without tracing all of these code paths to see why the defaults are not set in the command, I suppose we leave them be?

They're more like default behaviors (i.e. "use whatever the main branch is") rather than default values. The gh api --hostname instance however could be probably moved into an actual default value rather than in the usage text.

[zsloane]

Should I keep this updated with changes from main periodically while it's being reviewed?

[andyfeller]

Should I keep this updated with changes from main periodically while it's being reviewed?

Firstly, thank you for your patience with slow replies due to holiday leave! 🙇

Secondly, I hadn't forgotten about you. Let's see get reacquainted with where we left off to see if we can get this merged finally. 💪

Lastly, some PRs are okay not keeping them updating with the latest from trunk, this review is a little unique because of what we're trying to do. That said, let's try to wrap this up and get it shipped.

[andyfeller]

Using the following commands to locally build and compare the changes from this PR to the results in trunk:

gh pr checkout 8479
make manpages site-docs
grep -rn default site/ > ~/Desktop/8395-pr-site
grep -rn default share/man/ > ~/Desktop/8395-pr-man

git checkout trunk
git pull
make manpages site-docs
grep -rn default site/ > ~/Desktop/8395-trunk-site
grep -rn default share/man/ > ~/Desktop/8395-trunk-man

• 8395-pr-site.txt
• 8395-trunk-man.txt
• 8395-trunk-site.txt
• 8395-pr-man.txt

The main thing I think we need to figure out is how much consistency as there are several variations between the results and just looking at the code:

• (default "asc")
• (default: 30)
• (Default: ...)

[zsloane]

I can take a closer look at this a little later today but from my memory of the last time I was working on this, one source of the discrepancies are commands that use the usage string to tell the user about a default behavior (see examples below) rather than from the doc generation pulling it from the default value for the command (if that makes sense).

In your files it does look like the number defaults for limits and such still have the colon. I thought I already removed that. I will take a look

cli/pkg/cmd/release/create/create.go

Line 181 in 6309446

cmd.Flags().StringVar(&opts.Target, "target", "", "Target `branch` or full commit SHA (default: main branch)")

cli/pkg/cmd/gist/create/create.go

Line 99 in 6eb6501

cmd.Flags().BoolVarP(&opts.Public, "public", "p", false, "List the gist publicly (default: secret)")

cli/pkg/cmd/api/api.go

Line 229 in 6309446

cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "The GitHub hostname for the request (default \"github.com\")")

[zsloane]

For commands that have "default behaviors" in the usage strings, how should we handle it if we are going to be consistent and remove the colon?

cli/pkg/cmd/release/create/create.go

Line 181 in 6309446

cmd.Flags().StringVar(&opts.Target, "target", "", "Target `branch` or full commit SHA (default: main branch)")

cli/pkg/cmd/pr/checkout/checkout.go

Line 68 in 6309446

cmd.Flags().StringVarP(&opts.BranchName, "branch", "b", "", "Local branch name to use (default: the name of the head branch)")

Things like "Local branch name to use (default: the name of the head branch)" would turn into "Local branch name to use (default the name of the head branch)" which seems weird. I would suggest something like "Local branch name to use (default <the name of the head branch>)" but that might get confused with the typical syntax in man pages for optional arguments etc

[muzimuzhi]

(I'm from #8479.)

There are two default values belonging to settings of gh config https://cli.github.com/manual/gh_config whose docs are coded differently hence not yet covered in this PR.

cli/pkg/cmd/config/config.go

Lines 16 to 26 in 586eb8b

	func NewCmdConfig(f *cmdutil.Factory) *cobra.Command {
	longDoc := strings.Builder{}
	longDoc.WriteString("Display or change configuration settings for gh.\n\n")
	longDoc.WriteString("Current respected settings:\n")
	for _, co := range config.ConfigOptions() {
	longDoc.WriteString(fmt.Sprintf("- %s: %s", co.Key, co.Description))
	if co.DefaultValue != "" {
	longDoc.WriteString(fmt.Sprintf(" (default: %q)", co.DefaultValue))
	}
	longDoc.WriteRune('\n')
	}

[zsloane]

I will update the description of the default for gh config in this pr to match everywhere else

[andyfeller]

For commands that have "default behaviors" in the usage strings, how should we handle it if we are going to be consistent and remove the colon?

cli/pkg/cmd/release/create/create.go

Line 181 in 6309446

cmd.Flags().StringVar(&opts.Target, "target", "", "Target `branch` or full commit SHA (default: main branch)")

cli/pkg/cmd/pr/checkout/checkout.go

Line 68 in 6309446

cmd.Flags().StringVarP(&opts.BranchName, "branch", "b", "", "Local branch name to use (default: the name of the head branch)")

Things like "Local branch name to use (default: the name of the head branch)" would turn into "Local branch name to use (default the name of the head branch)" which seems weird. I would suggest something like "Local branch name to use (default <the name of the head branch>)" but that might get confused with the typical syntax in man pages for optional arguments etc

You raise a good point 🤔 In the case of "the name of the head branch", it seems this is also a little wordy and might be more concise "head branch name". Does changing the language make it less likely we need another special formatting syntax?

[zsloane]

Weird. My comment from after I committed my latest changes yesterday is just gone...?

Anyway, what I said was I opted for the (default <something>) syntax since that makes the most sense for now. I wanted to move the default formatting logic fully into the docs package but as this is my first time writing go I wasn't able to get the imports working etc. If someone wants to point me in the right direction for that I would be glad to keep going on it.

We already have the defval and options in ConfigOptions. If we just add the type then we can essentially do the same thing as getDefaultValueDisplayString. It could handle the (default ...) as well as the flag options {opt1|opt2|opt3} since these special cases like gh config don't show them

Does changing the language make it less likely we need another special formatting syntax?

I don't think so. Since the even something like "head branch name" isn't actually what you would pass to the flag. <something> is often used to mean <put the real value of **something**>

[andyfeller]

@zsloane: Think we're almost there! There is a regression from webpage formatting of keywords we need to knock out but I finished with comparing the results from these changes to man pages below

Thank you for your patience working through sudden changes in priority 🙇

Default changes for man pages

Note

cli/cli is your branch, cli/cli-trunk is trunk.

andyfeller@Andrews-MBP:cli/cli-trunk ‹trunk›$ for file in $(find . -type f -name "*.1"); do echo "\nFILE: $file\n"; diff <(man $file) <(man ../cli/$file); done

FILE: ./share/man/man1/gh-run-download.1

26c26
<        -D, --dir <string>
---
>        -D, --dir <string> (default ".")


FILE: ./share/man/man1/gh-search-issues.1

91c91
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
128c128
<        --order <string>
---
>        --order <string> (default "desc")
149c149
<        --sort <string>
---
>        --sort <string> (default "best-match")


FILE: ./share/man/man1/gh-auth-setup-git.1

33,37d32
<        -f, --force <--hostname>
<               Force setup even if the host is not known. Must be used in
<               conjunction with --hostname
< 
< 


FILE: ./share/man/man1/gh-api.1

116c116
<        -X, --method <string>
---
>        -X, --method <string> (default "GET")


FILE: ./share/man/man1/gh-search-prs.1

103c103
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
148c148
<        --order <string>
---
>        --order <string> (default "desc")
182c182
<        --sort <string>
---
>        --sort <string> (default "best-match")


FILE: ./share/man/man1/gh-extension-search.1

68c68
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
72c72
<        --order <string>
---
>        --order <string> (default "desc")
81c81
<        --sort <string>
---
>        --sort <string> (default "best-match")


FILE: ./share/man/man1/gh-workflow-list.1

33c33
<        -L, --limit <int>
---
>        -L, --limit <int> (default 50)


FILE: ./share/man/man1/gh-cache-list.1

24c24
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
28c28
<        -O, --order <string>
---
>        -O, --order <string> (default "desc")
32c32
<        -S, --sort <string>
---
>        -S, --sort <string> (default "last_accessed_at")


FILE: ./share/man/man1/gh-release-create.1

59,60c59
<               Mark this release as "Latest" (default: automatic based on date
<               and version)
---
>               Mark this release as "Latest" (default )
85c84
<               Target branch or full commit SHA (default: main branch)
---
>               Target branch or full commit SHA (default )


FILE: ./share/man/man1/gh-ssh-key-add.1

20c20
<        --type <string>
---
>        --type <string> (default "authentication")


FILE: ./share/man/man1/gh-run-watch.1

20c20
<        -i, --interval <int>
---
>        -i, --interval <int> (default 3)


FILE: ./share/man/man1/gh-codespace-list.1

33c33
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)


FILE: ./share/man/man1/gh-ruleset-view.1

35c35
<        -p, --parents
---
>        -p, --parents (default true)


FILE: ./share/man/man1/gh-pr-checkout.1

17c17
<               Local branch name to use (default: the name of the head branch)
---
>               Local branch name to use (default )


FILE: ./share/man/man1/gh-config.1

19,25c19,25
<        Current respected settings: - git_protocol: the protocol to use for git
<        clone and push operations (default: "https") - editor: the text editor
<        program to use for authoring text - prompt: toggle interactive
<        prompting in the terminal (default: "enabled") - pager: the terminal
<        pager program to send standard output to - http_unix_socket: the path
<        to a Unix socket through which to make an HTTP connection - browser:
<        the web browser to use for opening URLs
---
>        Current respected settings: - git_protocol: the protocol to use for git
>        clone and push operations {https|ssh} (default https) - editor: the
>        text editor program to use for authoring text - prompt: toggle
>        interactive prompting in the terminal {enabled|disabled} (default
>        enabled) - pager: the terminal pager program to send standard output to
>        - http_unix_socket: the path to a Unix socket through which to make an
>        HTTP connection - browser: the web browser to use for opening URLs


FILE: ./share/man/man1/gh-project-item-edit.1

61c61
<        --number <float32>
---
>        --number <float32> (default 0)


FILE: ./share/man/man1/gh-repo-sync.1

34c34
<               Branch to sync (default: default branch)
---
>               Branch to sync (default )


FILE: ./share/man/man1/gh-pr-checks.1

29c29
<        -i, --interval <--watch>
---
>        -i, --interval <--watch> (default 10)


FILE: ./share/man/man1/gh-gist-create.1

38c38
<               List the gist publicly (default: secret)
---
>               List the gist publicly (default "secret")


FILE: ./share/man/man1/gh-label-list.1

34c34
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
38c38
<        --order <string>
---
>        --order <string> (default "asc")
46c46
<        --sort <string>
---
>        --sort <string> (default "created")


FILE: ./share/man/man1/gh-project-field-list.1

24c24
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)


FILE: ./share/man/man1/gh-gist-list.1

16c16
<        -L, --limit <int>
---
>        -L, --limit <int> (default 10)


FILE: ./share/man/man1/gh-run-list.1

40c40
<        -L, --limit <int>
---
>        -L, --limit <int> (default 20)


FILE: ./share/man/man1/gh-release-list.1

24,32c24
<        -q, --jq <expression>
<               Filter JSON output using a jq expression
< 
< 
<        --json <fields>
<               Output JSON with the specified fields
< 
< 
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
34,37d25
< 
< 
<        -t, --template <string>
<               Format JSON output using a Go template; see "gh help formatting"


FILE: ./share/man/man1/gh-release-download.1

35c35
<        -D, --dir <directory>
---
>        -D, --dir <directory> (default ".")


FILE: ./share/man/man1/gh-codespace-delete.1

37c37
<        --days <N>
---
>        --days <N> (default 0)


FILE: ./share/man/man1/gh-run-view.1

16c16
<        -a, --attempt <uint>
---
>        -a, --attempt <uint> (default 0)


FILE: ./share/man/man1/gh-org-list.1

16c16
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)


FILE: ./share/man/man1/gh-repo-clone.1

37c37
<        -u, --upstream-remote-name <string>
---
>        -u, --upstream-remote-name <string> (default "upstream")


FILE: ./share/man/man1/gh-project-list.1

28c28
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)


FILE: ./share/man/man1/gh-variable-list.1

29,36d28
<        -q, --jq <expression>
<               Filter JSON output using a jq expression
< 
< 
<        --json <fields>
<               Output JSON with the specified fields
< 
< 
39,42d30
< 
< 
<        -t, --template <string>
<               Format JSON output using a Go template; see "gh help formatting"


FILE: ./share/man/man1/gh-search-code.1

52c52
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)

FILE: ./share/man/man1/gh-secret-set.1

69c69
<        -v, --visibility <string>
---
>        -v, --visibility <string> (default "private")


FILE: ./share/man/man1/gh-project-item-list.1

24c24
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)


FILE: ./share/man/man1/gh-search-repos.1

75c75
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
88c88
<        --order <string>
---
>        --order <string> (default "desc")
101c101
<        --sort <string>
---
>        --sort <string> (default "best-match")
149,151d148
< 
<        # search repositories excluding archived repositories
<        $ gh search repos --archived=false


FILE: ./share/man/man1/gh-ruleset-list.1

36c36
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
44c44
<        -p, --parents
---
>        -p, --parents (default true)


FILE: ./share/man/man1/gh-pr-create.1

79,80c79,80
<               The branch that contains commits for your pull request (default:
<               current branch)
---
>               The branch that contains commits for your pull request (default
>               )


FILE: ./share/man/man1/gh-repo-fork.1

56c56
<        --remote-name <string>
---
>        --remote-name <string> (default "origin")


FILE: ./share/man/man1/gh-codespace-ssh.1

92c92
<        --server-port <int>
---
>        --server-port <int> (default 0)


FILE: ./share/man/man1/gh-search-commits.1

75c75
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
83c83
<        --order <string>
---
>        --order <string> (default "desc")
100c100
<        --sort <string>
---
>        --sort <string> (default "best-match")


FILE: ./share/man/man1/gh-pr-diff.1

28c28
<        --color <string>
---
>        --color <string> (default "auto")


FILE: ./share/man/man1/gh-release-edit.1

47c47
<               Target branch or full commit SHA (default: main branch)
---
>               Target branch or full commit SHA (default )


FILE: ./share/man/man1/gh-variable-set.1

50c50
<        -v, --visibility <string>
---
>        -v, --visibility <string> (default "private")


FILE: ./share/man/man1/gh-pr-list.1

62c62
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
70c70
<        -s, --state <string>
---
>        -s, --state <string> (default "open")


FILE: ./share/man/man1/gh-issue-list.1

50c50
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)
66c66
<        -s, --state <string>
---
>        -s, --state <string> (default "open")


FILE: ./share/man/man1/gh-repo-list.1

46c46
<        -L, --limit <int>
---
>        -L, --limit <int> (default 30)

[andyfeller]

🤔 Couple of oddities when comparing the results above:

• gh auth setup-git

  $ diff <(man ./share/man/man1/gh-auth-setup-git.1) <(man ../cli/share/man/man1/gh-auth-setup-git.1)
  33,37d32
  <        -f, --force <--hostname>
  <               Force setup even if the host is not known. Must be used in
  <               conjunction with --hostname
  < 
  < 
  ``
  

• gh release create

  $ diff <(man ./share/man/man1/gh-release-create.1) <(man ../cli/share/man/man1/gh-release-create.1)
  59,60c59
  <               Mark this release as "Latest" (default: automatic based on date
  <               and version)
  ---
  >               Mark this release as "Latest" (default )
  85c84
  <               Target branch or full commit SHA (default: main branch)
  ---
  >               Target branch or full commit SHA (default )

• gh release edit

  $ diff <(man ./share/man/man1/gh-release-edit.1) <(man ../cli/share/man/man1/gh-release-edit.1)
  47c47
  <               Target branch or full commit SHA (default: main branch)
  ---
  >               Target branch or full commit SHA (default )

• gh pr create

  $ diff <(man ./share/man/man1/gh-pr-create.1) <(man ../cli/share/man/man1/gh-pr-create.1)
  79,80c79,80
  <               The branch that contains commits for your pull request (default:
  <               current branch)
  ---
  >               The branch that contains commits for your pull request (default
  >               )

[andyfeller]

@zsloane : thoughts about the anomalies in #8395 (comment)?

[zsloane]

@zsloane : thoughts about the anomalies in #8395 (comment)?

Still looking into the first one but for the rest I think I just need to escape the brackets?

cmdutil.NilBoolFlag(cmd, &opts.IsLatest, "latest", "", "Mark this release as \"Latest\" (default <automatic based on date and version>)")

[zsloane]

• gh auth setup-git

  $ diff <(man ./share/man/man1/gh-auth-setup-git.1) <(man ../cli/share/man/man1/gh-auth-setup-git.1)
  33,37d32
  <        -f, --force <--hostname>
  <               Force setup even if the host is not known. Must be used in
  <               conjunction with --hostname
  < 
  < 
  ``

Ah, ok. I see. This is just due to a change that hasn't been synced onto my fork yet

[zsloane]

Anyone have thoughts on this:

cmdutil.NilBoolFlag(cmd, &opts.IsLatest, "latest", "", "Mark this release as \"Latest\" (default <automatic based on date and version>)")

Anything in between the < > just gets stripped from the string...

[andyfeller]

Anyone have thoughts on this:

cmdutil.NilBoolFlag(cmd, &opts.IsLatest, "latest", "", "Mark this release as \"Latest\" (default <automatic based on date and version>)")

Anything in between the < > just gets stripped from the string...

@zsloane : If the brackets are problematic, perhaps we don't use the brackets

[zsloane]

Anyone have thoughts on this:

cmdutil.NilBoolFlag(cmd, &opts.IsLatest, "latest", "", "Mark this release as \"Latest\" (default <automatic based on date and version>)")

Anything in between the < > just gets stripped from the string...

@zsloane : If the brackets are problematic, perhaps we don't use the brackets

I'm good with whatever. The brackets make the most sense to me in the context but we can do either of the following:

1. Just remove them. This example would become ... (default automatic based on date and version) which gets the point across but isn't consitent.
2. I could find some other character that works but isn't quite right for the context. Probably [something] or (something). * - are two others that don't work

I just got back from a work trip so I can spend some more time looking at this now.

[zsloane]

I went with the ... (default [automatic based on date and version]) option for now so we can hopefully get this closed out.

It doesn't look quite right but maybe it is good enough for now

[andyfeller]

I went with the ... (default [automatic based on date and version]) option for now so we can hopefully get this closed out.

It doesn't look quite right but maybe it is good enough for now

Thank you once again, @zsloane, for your patience given how nuanced these changes turned out and limited capacity of myself. 🙇 ❤️ I definitely want to see these changes get shipped, so I'm devoting time today to get these cleared.