patterngogitMinor
Breakable Toy: Go CLI for Gitlab
Viewed 0 times
toygitlabbreakableforcli
Problem
I started to learn programming in Go and came up with a Breakable Toy, which is a CLI for Gitlab. To get something up and running fast, I used some Go libraries:
The project can be found in this repository on Github.
I am looking for any feedback / code review that could help me to better understand the concepts and conventions of Go. I come from the Java world and have the impression that I try to stick too much on the object oriented world. So any feedback concerning code style and best-practices is highly appreciated.
Besides general feedback, here are some specific questions:
Being used to the
so there are really many LOCs only for error handling - is that the "way to go"?
The Cobra library seems to store values for CLI flags in global variables:
This makes it really hard to figure out which flags belong to wh
- Ginkgo for testing
- Cobra for basic CLI stuff
- Sling for HTTP requests and JSON mapping
The project can be found in this repository on Github.
I am looking for any feedback / code review that could help me to better understand the concepts and conventions of Go. I come from the Java world and have the impression that I try to stick too much on the object oriented world. So any feedback concerning code style and best-practices is highly appreciated.
Besides general feedback, here are some specific questions:
- Error Handling in Go
Being used to the
try catch mechanism in Java, the _, err := mechanism in Go seems a little unhandy to me. To me it looks as if I have to implement the "bubble up" of exceptions all by myself. Example:// HTTP client
func (client *GitlabClient) Do(req *http.Request, value interface{}) (*http.Response, error) {
resp, err := client.sling.Do(req, value, nil)
return resp, err
}
// service
func (service *ProjectsService) List() (*[]model.Project, error) {
// ...
_, err = service.Client.Do(req, projects)
if err != nil {
return nil, err
}
return projects, nil
}
// CLI command
func(cmd *cobra.Command, args []string) error {
projects, err := gitlabClient.Projects.List()
if err != nil {
return err
}
err = OutputJson(projects)
return err
}so there are really many LOCs only for error handling - is that the "way to go"?
- Global Variables
The Cobra library seems to store values for CLI flags in global variables:
var id string
// ...
projectGetCmd.PersistentFlags().StringVarP(&id, "id", "i", "", "(required) Either ID of project or 'namespace/project-name'")
viper.BindPFlag("id", projectGetCmd.PersistentFlags().Lookup("id"))This makes it really hard to figure out which flags belong to wh
Solution
- Yes, this is the way to do error handling in Go. By design, you can't let exceptions propagate to the callers "silently", you have to consider what makes sense at each step. I found that on large-scale projects, it makes it significantly easier to predict and test the error handling behavior of your code. It also forces you to put the error handling first, which (imho) results in more readable, less nested code.
- Truly global variables don't exist in Go — if a package
fooexports a variableBar, other packages will have to callfoo.Barto access it. That being said, it pretty much never makes sense to do that.
In the example you gave, cmd.go and project.go are in the same package, and all package-level variables are shared, so identifiers conflict. If you want this variable to be shared, it's more readable to put all functions that use it in the same file.
- When you want to mock some of your functions, it's the sign that there should probably be an associated interface. You can then "mock" an interface easily, by re-implementing your interface in your tests and making it do whatever you want. I find this more readable and idiomatic than using complex frameworks.
Note on the last question: if your interface has many functions:
type Fooer interface {
Foo1(string) (string, error)
Foo2(int) (string, int)
...
// Foo42() []byte
}and you only want to test
Foo1, you can use the following syntaxtype fakeFooer struct {
Fooer
ret string
}
func (fi fakeFooer) Foo1(_ string) (string, error) {
return fi.ret, nil
}and not implement any other functions.
fakeFooer{"bar"} will "implement" Fooer, calling Foo1 will do what you expect, and trying to call Foo2 on it will panic (nil pointer dereference).Code Snippets
type Fooer interface {
Foo1(string) (string, error)
Foo2(int) (string, int)
...
// Foo42() []byte
}type fakeFooer struct {
Fooer
ret string
}
func (fi fakeFooer) Foo1(_ string) (string, error) {
return fi.ret, nil
}Context
StackExchange Code Review Q#157673, answer score: 2
Revisions (0)
No revisions yet.