Skip to content

gh alias list - #991

Merged
vilmibm merged 4 commits into
trunkfrom
alias-list
Jun 5, 2020
Merged

vilmibm merged 4 commits into
trunkfrom
alias-list

Conversation

@vilmibm

@vilmibm vilmibm commented May 22, 2020 •

Copy link
Copy Markdown
Contributor

image

Fixes #989
Part of #936

@mislav
mislav changed the base branch from master to trunk May 27, 2020 11:41
@fares378

This comment has been minimized.

@cli cli temporarily blocked fares378 Jun 3, 2020
@vilmibm
vilmibm marked this pull request as ready for review June 3, 2020 22:29
@vilmibm vilmibm changed the title [WIP] gh alias list gh alias list Jun 3, 2020

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Neat!

Comment thread internal/config/alias_config.go Outdated
Comment on lines +53 to +54
for i := 0; i < len(a.Root.Content); i += 2 {
if i+1 == len(a.Root.Content) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could the condition inside the for loop be adjusted so that we don't need this extra check?

Suggested change
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 {

Comment thread command/alias_test.go Outdated
Comment on lines +289 to +294
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'")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread command/alias.go Outdated
sort.Strings(keys)

for _, alias := range keys {
tp.AddField(alias+":", nil, nil)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 #.

Comment thread command/alias.go Outdated
Comment on lines +122 to +124
Example: `$ gh alias list
co: pr checkout
bugs: issue list --label="bugs"`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread command/alias.go
for alias := range aliasMap {
keys = append(keys, alias)
}
sort.Strings(keys)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice touch with sorting keys!

@vilmibm
vilmibm merged commit 494598a into trunk Jun 5, 2020
@mislav
mislav deleted the alias-list branch June 25, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gh alias list

3 participants