HiveBrain v1.2.0
Get Started
← Back to all entries
patterngogitMinor

Breakable Toy: Go CLI for Gitlab

Submitted by: @import:stackexchange-codereview··
0
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:

  • 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:

  1. 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"?

  1. 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 foo exports a variable Bar, other packages will have to call foo.Bar to 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 syntax

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