Conversation
This comment has been minimized.
This comment has been minimized.
internal/config/alias_config.go
Outdated
| for i := 0; i < len(a.Root.Content); i += 2 { | ||
| if i+1 == len(a.Root.Content) { |
There was a problem hiding this comment.
Could the condition inside the for loop be adjusted so that we don't need this extra check?
| for i := 0; i < len(a.Root.Content); i += 2 { | |
| if i+1 == len(a.Root.Content) { | |
| for i := 0; i < len(a.Root.Content)-1; i += 2 { |
command/alias_test.go
Outdated
| test.ExpectLines(t, output.String(), | ||
| "clone:\trepo clone", | ||
| "co:\tpr checkout", | ||
| "il:\tissue list --author=\\$1 --label=\\$2", | ||
| "prs:\tpr status", | ||
| "cs:\tconfig set editor 'quoted path'") |
There was a problem hiding this comment.
Since the output of this command should be easy to assert as a whole, I think we don't need ExpectLines here and can instead just compare output.String() to a static string. This has the added benefit of asserting that the lines appear in the right order as well.
command/alias.go
Outdated
| sort.Strings(keys) | ||
|
|
||
| for _, alias := range keys { | ||
| tp.AddField(alias+":", nil, nil) |
There was a problem hiding this comment.
When the table is output to a pipe instead of to a terminal (tb.IsTTY()), we could omit the : so that the script can split by \t and end up with key/value pairs that need no extra processing. We already do that with issues/PRs: when piping to a script, we output issue numbers without the #.
command/alias.go
Outdated
| Example: `$ gh alias list | ||
| co: pr checkout | ||
| bugs: issue list --label="bugs"`, |
There was a problem hiding this comment.
I would offer here that, in general, we don't need to provide examples of invocations that take no arguments. The Usage line already covers that
| for alias := range aliasMap { | ||
| keys = append(keys, alias) | ||
| } | ||
| sort.Strings(keys) |
There was a problem hiding this comment.
Nice touch with sorting keys!
Fixes #989
Part of #936