list the various alias permutations for the command and subcommands, via '--help' and 'gh reference' #8824
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cliAutomation
added
the
external
gabemontero
commented
Mar 16, 2024
pkg/cmd/root/help_reference_test.go
Outdated
| @@ -0,0 +1,27 @@ | |||
| package root | |||
There was a problem hiding this comment.
this file was soley for ease of use testing purposes really, though technically we are validating some basic errors are not occurring on command construction and help generation.
I'm fine removing it if you like
|
Thanks for opening up this PR and contributing to the GitHub CLI, @gabemontero! ❤️ 🤔 Is there an easier format include alias information while making it easier to read?Building the changes locally, there are a few concerns that come to mind:
### `gh [codespace|cs] list [flags]`
List codespaces
-q, --jq expression Filter JSON output using a jq expression
--json fields Output JSON with the specified fields
-L, --limit int Maximum number of codespaces to list (default 30)
-o, --org login The login handle of the organization to list codespaces for (admin-only)
-R, --repo string Repository name with owner: user/repo
-t, --template string Format JSON output using a Go template; see "gh help formatting"
-u, --user username The username to list codespaces for (used with --org)
-w, --web List codespaces in the web browser, cannot be used with --user or --org$ ./bin/gh cs ls --help
List codespaces of the authenticated user.
Alternatively, organization administrators may list all codespaces billed to the organization.
For more information about output formatting flags, see `gh help formatting`.
USAGE
gh codespace list [flags]
ALIASES
ls
FLAGS
-q, --jq expression Filter JSON output using a jq expression
--json fields Output JSON with the specified fields
-L, --limit int Maximum number of codespaces to list (default 30)
-o, --org login The login handle of the organization to list codespaces for (admin-only)
-R, --repo string Repository name with owner: user/repo
-t, --template string Format JSON output using a Go template; see "gh help formatting"
-u, --user username The username to list codespaces for (used with --org)
-w, --web List codespaces in the web browser, cannot be used with --user or --org
INHERITED FLAGS
--help Show help for command
LEARN MORE
Use `gh <command> <subcommand> --help` for more information about a command.
Read the manual at https://cli.github.com/manualTossing around ideasShould we avoid the complexity of having command aliases show up as I mention this in the issue but I think trying to get all aliases to show up like other command ### `gh codespace list [flags]`
List codespaces
ALIASES
gh codespace ls
gh cs ls
FLAGS
-q, --jq expression Filter JSON output using a jq expression
--json fields Output JSON with the specified fields |
|
Gentle nudge @gabemontero, looks like Andy left some thoughts for you if you're still interested, otherwise please close so someone else can pick it up. Cheers! |
yeah @andyfeller and I talked a bit offline @williammartin .... unfortunately I simply have had no time to deal with this so far with my regular job. How about if I can't get back to it this week, I'll close it. That sound OK? thanks |
|
Cool, I totally understand. You can also close it now and reopen it whenever you feel like you would have time to contribute if no one has picked it up. Whatever you feel better captures your intent. Thanks! |
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
from
efcee53
to
cc0d251
Compare
|
ok @andyfeller @williammartin I've redone things based on @andyfeller 's #8824 (comment) Log snippet: I'll redo the PR title in a sec since it does not affect |
gabemontero
changed the title
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
2 times, most recently
from
98476f9
to
3b6c957
Compare
|
@williammartin : how do you feel about the Latest release$ gh codespace --help
Connect to and manage codespaces
USAGE
gh codespace [flags]
ALIASES
cs
...$ gh codespace list --help
List codespaces of the authenticated user.
Alternatively, organization administrators may list all codespaces billed to the organization.
For more information about output formatting flags, see `gh help formatting`.
USAGE
gh codespace list [flags]
ALIASES
ls
...Changes under test$ ./bin/gh codespace --help
Connect to and manage codespaces
USAGE
gh codespace [flags]
ALIASES
gh cs
...$ ./bin/gh codespace list --help
List codespaces of the authenticated user.
Alternatively, organization administrators may list all codespaces billed to the organization.
For more information about output formatting flags, see `gh help formatting`.
USAGE
gh codespace list [flags]
ALIASES
gh codespace ls
gh cs ls
... |
|
I think the only thing I'd find weird is if we had a parent and a subcommand both with more than one alias, it gets pretty gnarly. For example, adding a new alias to I think that's kinda bad? It's not something we have right now though. |
@williammartin: How would you recommend proceeding? I agree IF we get into such a situation of parent and child having multiple aliases this is going to look 🤢. Offhand, 4 options come to mind:
|
|
I think I'd favour |
|
I could do WDYT @williammartin @andyfeller ? I'll see about squeezing in a few minutes this week to do it. |
|
Sounds good to me thanks. Note that if we don't include the parent we also need to drop the |
|
wait ... just refreshed my memory with the code diff, and When I reverted my change: or am I missing some nuance wrt what |
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
from
3b6c957
to
d254399
Compare
ah, let me see if I can take a step back as there are 2 areas being discussed:
With that said, I think there are 2 questions:
|
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
2 times, most recently
from
36068e3
to
6580b36
Compare
|
thanks @andyfeller those clarifications helped alot here's my initial take, where for you question 1 at the end, I took the liberty of creating a comma separated list, where the "unaliased" i.e. "original" item is listed first. PTAL |
|
if we are all in agreement I'll adjust the PR title and description again, and add back the fixes 8824 |
|
Made a few minor changes, which I'll demonstrate here that I think should finish this up: 1. Removed unaliased command name from showing up under aliases $ ./bin/gh codespace list --help
List codespaces of the authenticated user.
Alternatively, organization administrators may list all codespaces billed to the organization.
For more information about output formatting flags, see `gh help formatting`.
USAGE
gh codespace list [flags]
ALIASES
gh codespace ls, gh cs ls$ ./bin/gh reference
gh reference
...
## gh codespace
Connect to and manage codespaces
Aliases
gh cs
...
### gh codespace list [flags]
List codespaces
-q, --jq expression Filter JSON output using a jq expression
--json fields Output JSON with the specified fields
-L, --limit int Maximum number of codespaces to list (default 30)
-o, --org login The login handle of the organization to list codespaces for (admin-only)
-R, --repo string Repository name with owner: user/repo
-t, --template string Format JSON output using a Go template; see "gh help formatting"
-u, --user username The username to list codespaces for (used with --org)
-w, --web List codespaces in the web browser, cannot be used with --user or --org
Aliases
gh codespace ls, gh cs ls2. Removed "No aliases" showing up for commands without aliases
### gh codespace logs [flags]
Access codespace logs
-c, --codespace string Name of the codespace
-f, --follow Tail and follow the logs
-R, --repo string Filter codespace selection by repository name (user/repo)
--repo-owner string Filter codespace selection by repository owner (username or org)
Thoughts? |
gabemontero
commented
May 10, 2024
pkg/cmd/root/help.go
Outdated
| @@ -311,10 +311,6 @@ func buildAliases(cmd *cobra.Command, aliasList []string) string { | |||
| return strings.Join(aliasList, " ") | |||
| } | |||
|
|
|||
| if cmd.Parent().HasParent() { | |||
| // prepend cmd.Name so unaliased is first | |||
| aliasList = append([]string{cmd.Name()}, aliasList...) | |||
There was a problem hiding this comment.
removes the atypical/clever prepend bit, but I get it ;-)
pkg/cmd/root/help.go
Outdated
|
|
||
| func buildAliases(cmd *cobra.Command, aliasList []string) string { | ||
| if len(aliasList) == 0 { | ||
| return "No Aliases" |
There was a problem hiding this comment.
couldn't tell from your point number 2 - Removed "No aliases" showing up for commands without aliases if that was just for gh reference or if you did not want that displayed for gh <cmd> [<subcmd>...] --help as well.
There was a problem hiding this comment.
Ah sorry, yes that was making gh reference not show Aliases if there were no aliases like --help. Otherwise it looked like the following:
### gh variable set <variable-name> [flags]
Create or update variables
-b, --body string The value for the variable (reads from standard input if not specified)
-e, --env environment Set deployment environment variable
-f, --env-file file Load variable names and values from a dotenv-formatted file
-o, --org organization Set organization variable
-r, --repos repositories List of repositories that can access an organization variable
-v, --visibility string Set visibility for an organization variable: {all|private|selected} (default "private")
Aliases
No Aliases
## gh workflow <command>
View details about GitHub Actions workflows
Aliases
No Aliases
### gh workflow disable [<workflow-id> | <workflow-name>]
Disable a workflow
Aliases
No Aliases
### gh workflow enable [<workflow-id> | <workflow-name>]
Enable a workflow
Aliases
No Aliases
|
@gabemontero : definitely want your thoughts and 👍 before just accepting my own changes and merging 😅 |
|
So running with your commit in the debugger @andyfeller the line https://github.com/cli/cli/pull/8824/files#diff-12d8f41408bbd28c0b811bfd37e246792395d8313d6da8c6b2dbf281b98b923dR308 never gets hit even with running help on say I'm going to rebase onto latest trunk and remove that if check and return of See if my change still looks OK to you. Also, I think we are back into fixing #8677 territory no? If so, I'll add the fixes designation in the PR description. |
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
from
8751069
to
edf55d3
Compare
|
ok third commit pushed @andyfeller |
gabemontero
changed the title
|
also adjusted PR title and description unless we have more iterating to do once @williammartin sees this, do you all typically squash commits when it is all said and done @andyfeller ? |
|
Looks good! andyfeller@Andys-MBP:cli/cli ‹ggm-issue-8677-tinker›$ ./bin/gh cs --help
Connect to and manage codespaces
USAGE
gh codespace [flags]
ALIASES
gh cs
...andyfeller@Andys-MBP:cli/cli ‹ggm-issue-8677-tinker›$ ./bin/gh cs list --help
List codespaces of the authenticated user.
Alternatively, organization administrators may list all codespaces billed to the organization.
For more information about output formatting flags, see `gh help formatting`.
USAGE
gh codespace list [flags]
ALIASES
gh codespace ls, gh cs ls
...andyfeller@Andys-MBP:cli/cli ‹ggm-issue-8677-tinker›$ ./bin/gh reference
gh reference
...
### gh codespace list [flags]
List codespaces
-q, --jq expression Filter JSON output using a jq expression
--json fields Output JSON with the specified fields
-L, --limit int Maximum number of codespaces to list (default 30)
-o, --org login The login handle of the organization to list codespaces for (admin-only)
-R, --repo string Repository name with owner: user/repo
-t, --template string Format JSON output using a Go template; see "gh help formatting"
-u, --user username The username to list codespaces for (used with --org)
-w, --web List codespaces in the web browser, cannot be used with --user or --org
Aliases
gh codespace ls, gh cs ls |
🤔 we allow squashing merging but it isn't a hard and fast "thou must". |
roger that .... let's sit tight and see what @williammartin says then wrt the format here thanks @andyfeller |
pkg/cmd/root/help.go
Outdated
| @@ -302,3 +302,28 @@ func dedent(s string) string { | |||
| } | |||
| return strings.TrimSuffix(buf.String(), "\n") | |||
| } | |||
|
|
|||
| func buildAliases(cmd *cobra.Command, aliasList []string) string { | |||
There was a problem hiding this comment.
I think we'll make our lives easier if we remove the string formatting from here and only build up the list of aliases, then format it with strings.Join like so:
func buildAliasList(cmd *cobra.Command, aliases []string) []string {
if !cmd.HasParent() {
return aliases
}
parentAliases := append(cmd.Parent().Aliases, cmd.Parent().Name())
sort.Strings(parentAliases)
var aliasesWithParentAliases []string
// e.g aliases = [ls]
for _, alias := range aliases {
// e.g parentAliases = [codespaces, cs]
for _, parentAlias := range parentAliases {
// e.g. aliasesWithParentAliases = [codespaces list, codespaces ls, cs list, cs ls]
aliasesWithParentAliases = append(aliasesWithParentAliases, fmt.Sprintf("%s %s", parentAlias, alias))
}
}
return buildAliasList(cmd.Parent(), aliasesWithParentAliases)
}And at the call site:
strings.Join(buildAliasList(cmd, cmd.Aliases), ", "))There was a problem hiding this comment.
But it is kind of late here and I might have missed some implementation detail!
There was a problem hiding this comment.
I don't think you missed anything @williammartin ... your simplifications look good to me wrt
gh reference--helpwhen there are no aliases, single aliases, multilple aliases
|
Spoke with Andy in Slack. I don't mind too much about stylistic things because they are easy to iterate on. In this case I would recommend that in Before
After
|
|
Secondly, we should make sure this change is also reflected in the generated manpages and website, which it currently isn't. A recent example of this was done here https://github.com/cli/cli/pull/8934/files#diff-33caf7ff0fd1f656c40c32b2a64616cce45fa04e225e201a39cafb689db6a779 |
|
it might be a day or two but I should be able to process @williammartin 's latest items by the end of the week |
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
from
edf55d3
to
0d4216f
Compare
|
updates pushed @williammartin @andyfeller for both the code simplifications and generated man/web content |
this occurs from either utlizing the help function for a specific command/subcommand combination or when 'gh reference' lists the command tree
This commit adds conditional logic in `gh reference` to only show aliases if there are aliases to see. Additionally, this removes the unaliased command name from being shown under aliases.
gabemontero
force-pushed
the
ggm-issue-8677-tinker
branch
from
0d4216f
to
39e4cbd
Compare
Manual testing notes
|
andyfeller
approved these changes
May 17, 2024
There was a problem hiding this comment.
Huge thanks, @gabemontero, for your contribution and patience! 😸
likewise thanks @andyfeller @williammartin for the attention and patience :-) it was nice to dust off my golang cli skills |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hey @andyfeller !!
After our recent catch up over lunch, I finally got some time to check out some of the github CLI stuff you were involved with, and noticed a couple of open issues you had filed, and decided to take a crack at coding up a fix for one of them to get better acclimated with things! The one that caught my eye ....
Fixes #8677
[Editor note: there were a couple of pivots through the course of the PR on what to do exactly. Final screen shots on output are the last few comments]