Reconsider the return types of certain API functions

[marpaia]

Many functions, such as chef.GetClient(string), return three values:

• client chef.Client (or whatever)
• err error
• ok bool

if err != nil, something with the request went wrong.

if err == nil and !ok, then the request was successful and you were able to speak to the chef server properly, but the thing you were requesting (in this case a client) didn't exist on the server.

if err == nil and ok == true, then the request succeeded and your client data is stored in the client variable.

question

is this too obtuse? i don't think so personally, but I'd like @spheromak's opinion on this, as i've been wondering lately.

[marpaia]

@spheromak i'm assigning this to you because this is mainly here so that i can hear your thoughts on the subject

[marpaia]

ie: consider the following database/sql idiom:

package main

import (
    "database/sql"
    _ "github.com/go-sql-driver/mysql"
    "log"
)

func main() {
    db, err := sql.Open("mysql",
        "user:password@tcp(127.0.0.1:3306)/test")
    if err != nil {
        log.Println(err)
    }

    var str string
    err = db.QueryRow(
        "select hello from world where id = 1").
        Scan(&str)
    if err != nil && err != sql.ErrNoRows {
        log.Println(err)
    }
    log.Println(str)

    defer db.Close()
}

Notice the line: if err != nil && err != sql.ErrNoRows

We could refactor to do something like if err != nil && err != sql.NoData?

This would break a lot of the package though. May not be worth it for that reason alone.

[marpaia]

Some thoughts on this:

For context, this is how the package would currently be used now:

package main

import (
    "fmt"
    "os"

    "github.com/marpaia/chef-golang"
)

var c *chef.Chef

func init() {
    var err error
    c, err = chef.Connect()
    if err != nil {
        fmt.Println("Error:", err)
        os.Exit(1)
    }
}

func main() {
    cookbookName := "neo4j"
    cookbookVersion := "1.0"
    currentVersion, ok, err := c.GetCookbookVersion(cookbookName, cookbookVersion)
    if err != nil {
        // there was a problem communicating with the server, exit the program
        fmt.Println("Error communicating with the Chef server:", err)
        os.Exit(1)
    }
    if !ok {
        // the request was successful but the cookbook requested doesn't exist
        fmt.Printf("Cookbook %s Version %s doesn't exist.\n", cookbookName, cookbookVersion)
    } else {
        // the request was successful and the cookbook requested does exist
        // print out the files in the cookbook
        if len(currentVersion.Files) > 0 {
            fmt.Println("\n  [+]", cookbookName, currentVersion.Version, "Cookbook Files")
            for _, cookbookFile := range currentVersion.Files {
                fmt.Println("    - ", cookbookFile.Name)
            }
        }
    }
}

As you can see, you have to handle if err is nil and if ok is true separately. My initial logic there was that it would force people to consider both cases individually, but perhaps that's burdensome.

The alternative would be something like:

package main

import (
    "fmt"
    "os"

    "github.com/marpaia/chef-golang"
)

var c *chef.Chef

func init() {
    var err error
    c, err = chef.Connect()
    if err != nil {
        fmt.Println("Error:", err)
        os.Exit(1)
    }
}

func main() {
    cookbookName := "neo4j"
    cookbookVersion := "1.0"
    currentVersion, err := c.GetCookbookVersion(cookbookName, cookbookVersion)
    if err != nil && err != chef.NoDataError {
        fmt.Println("Error retreiving the cookbook:", err)
        os.Exit(1)
    }
    // the request was successful and the cookbook requested does exist
    // print out the files in the cookbook
    if len(currentVersion.Files) > 0 {
        fmt.Println("\n  [+]", cookbookName, currentVersion.Version, "Cookbook Files")
        for _, cookbookFile := range currentVersion.Files {
            fmt.Println("    - ", cookbookFile.Name)
        }
    }
}

I feel as though hiding the error like that would make how you're supposed to handle the values a little less obvious.

Also, since ok will only ever be true if the request was successful AND you got results, you could theoretically underscore the error, but still check the errors existence. Consider:

package main

import (
    "fmt"
    "os"

    "github.com/marpaia/chef-golang"
)

var c *chef.Chef

func init() {
    var err error
    c, err = chef.Connect()
    if err != nil {
        fmt.Println("Error:", err)
        os.Exit(1)
    }
}

func main() {
    cookbookName := "neo4j"
    cookbookVersion := "1.0"
    currentVersion, ok, _ := c.GetCookbookVersion(cookbookName, cookbookVersion)
    if !ok {
        fmt.Println("Error retreiving the cookbook")
        os.Exit(1)
    }
    // the request was successful and the cookbook requested does exist
    // print out the files in the cookbook
    if len(currentVersion.Files) > 0 {
        fmt.Println("\n  [+]", cookbookName, currentVersion.Version, "Cookbook Files")
        for _, cookbookFile := range currentVersion.Files {
            fmt.Println("    - ", cookbookFile.Name)
        }
    }
}

The above example doesn't require an API change and you kind of get the same functionality, except you don't get information about the error.

[spheromak]

All my issues are also the same. up there for comment @marpaia. I am all for building error types, and returning them.

I also want to convert the 'middleware' methods to take the same func signature Get/Put/Post should take the same sig (path, params, body). In the get_refactor branch @2592b18cab782cc68591d21601f3e3e2fbab0bcc I converted all of the chef.Get calls to use that sig but i didn't want to merge it without discussion.

[marpaia]

I'm down with the get refactor.

[spheromak]

see #28 for more ideas on #22