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

Add --list flag to open command #129

Merged
merged 1 commit into from Jun 20, 2018

Conversation

manumilou
Copy link
Collaborator

@manumilou manumilou commented Jun 15, 2018

Why

Add the --list flag support for the open command.

The --list command modifier lists in the standard output the entries found in the open section of dev.yml configuration file.

Resolves #111

How

Using Cobra local Flags primitive.

pkg/cmd/open.go Outdated
}

func init() {
openCmd.Flags().Bool("list", false, "List available project's URLs")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is the wrong option type

@@ -51,3 +51,10 @@ func FindLink(proj *project.Project, linkName string) (url string, err error) {

return
}

func PrintLinks(proj *project.Project) {
var url, title string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we get rid of this declaration?

func PrintLinks(proj *project.Project) {
var url, title string
for title, url = range proj.Manifest.Open {
fmt.Println(title, url)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Better formatting ?

pkg/cmd/open.go Outdated
@@ -1,17 +1,23 @@
package cmd

import (
"os"
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

if GetFlagBool(cmd, "list") {
err = open.PrintLinks(proj)
checkError(err)
os.Exit(0)
Copy link
Member

@pior pior Jun 15, 2018

Choose a reason for hiding this comment

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

checkError already exits with 1 (which may not be optimal in this case, but we can live with that I think)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there is no error though, we need to exit properly. Otherwise, we'll fail on the next error Error: which link should I open?.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the noise... 🤦‍♂️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no worries!

}
var url, title string
for title, url = range proj.Manifest.Open {
fmt.Println(title, url)
Copy link
Member

Choose a reason for hiding this comment

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

We could use \t as a limited attempt to align the columns.

@manumilou manumilou changed the title [WIP] Cmd open add list option Add --list flag to open command Jun 15, 2018
Copy link
Member

@pior pior left a comment

Choose a reason for hiding this comment

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

❤️

}

return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

This function is not really in the scope of the open helper.
It's fine to keep it here for now, but the fuzzy search adds some more code related to links, I think we should create a links helper for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you're right. Knowing how to print a link is not really the concern of the open helper.

Copy link
Member

Choose a reason for hiding this comment

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

Incremental changes! Let's ship that and keep it in mind!

Copy link
Collaborator

@mlhamel mlhamel left a comment

Choose a reason for hiding this comment

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

💯

@manumilou manumilou merged commit 915e0e8 into devbuddy:master Jun 20, 2018
@manumilou manumilou deleted the cmd-open-add-list-option branch June 20, 2018 20:00
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.

None yet

3 participants