Skip to content

gh alias delete#1103

Merged
vilmibm merged 2 commits intotrunkfrom
alias-delete
Jun 5, 2020
Merged

gh alias delete#1103
vilmibm merged 2 commits intotrunkfrom
alias-delete

Conversation

@vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Jun 4, 2020

closes #990

This PR adds gh alias delete <alias>. It's pretty straightforward.

$ gh alias delete foo
✓ Deleted alias foo; was issue list

Part of #936
See also #989

@vilmibm vilmibm added the aliases label Jun 4, 2020
@vilmibm vilmibm requested review from mislav and probablycorey June 4, 2020 18:01
@vilmibm vilmibm force-pushed the alias-delete branch 2 times, most recently from dc76fe0 to df0756f Compare June 4, 2020 18:18
Copy link
Contributor

@probablycorey probablycorey left a comment

Choose a reason for hiding this comment

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

👍 Looks great!

command/alias.go Outdated
return fmt.Errorf("no such alias %s", alias)
}

expansion := aliasCfg.Get(alias)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering what the return value would be if you didn't check for existence on line 137. This got me thinking about making the Get method work more like the Go map lookup. So the interface would be:

func Get(alias string) (value string, found bool)

This isn't something we have to do, but it feels like it might be more standard Go style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would make sense 👍

content := cm.Root.Content
for i := 0; i < len(content); i++ {
if content[i].Value == key {
i++ // skip the next node which is this key's value
Copy link
Contributor

Choose a reason for hiding this comment

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

This confused me, but I think it's mostly because iterating over YAML nodes is weird. But from what I can find this is just how it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadly yes this is how it works 😅

@vilmibm vilmibm merged commit 59580e6 into trunk Jun 5, 2020
@mislav mislav deleted the alias-delete 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 delete

2 participants