Open marpaia opened 10 years ago
@spheromak i'm assigning this to you because this is mainly here so that i can hear your thoughts on the subject
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.
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.
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.
I'm down with the get refactor.
see #28 for more ideas on #22
Many functions, such as chef.GetClient(string), return three values:
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.