Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh alias delete #1103

Merged
merged 2 commits into from Jun 5, 2020
Merged

gh alias delete #1103

merged 2 commits into from Jun 5, 2020

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 added this to Backlog 🗒 in The GitHub CLI via automation Jun 4, 2020
@vilmibm vilmibm moved this from Backlog 🗒 to Needs review 🤔 in The GitHub CLI Jun 4, 2020
@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 😅

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Jun 5, 2020
@vilmibm vilmibm merged commit 59580e6 into trunk Jun 5, 2020
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Jun 5, 2020
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Jun 11, 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
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

gh alias delete
2 participants