Proposal: Style Guide

[bufdev]

This is going to be a running list over the next week or so, but to kick it off, here's some proposals:

• All packages have a file named name_of_package.go, ie in api/server, we have server.go
• Packages are named after their directory, ie api/ is package api
• I'm 50/50 on this: sub-packages (sub-directories) concatenate their parent package (parent directory) name with their name to make their package name, ie api/server is package apiserver, and then we will have apiserver.go.
• ALL PUBLIC TYPES GO IN name_of_package.go. Every other file is just a helper file that implements the types.
• Structs without a corresponding interface are data holders. Structs with functions attached have a public interface wrapper, and then the struct becomes private. Example:

// foo.go
package foo

type Runner interace {
  Run(one string, i int) error
}

func NewRunner(something bar.Something) Runner {
  return newRunner(something)
}

// runner.go
package foo

type runner struct{
  something bar.Something
}

func newRunner(something bar.Something) *runner {
  return &runner{something}
}

func (r *runner) Run(one string, i int) error {
  r.hello(i)
  return r.something.Bar(one, i+1)
}

func (r *runner) hello(i int) {
  return r.something.Hello(i)
}

• Most structs that have functions attached have a separate file with the private struct definition, private constructor, and public functions, then private functions. The runner struct above is an example.
• Code should pass go vet, errcheck, golint #88 and Comments on packages and public types should be GoDoc friendly #97 (go vet, errcheck, golint)
• Remove all calls to logrus.Fatal{f,ln}, logrus.Panic{f,ln} #87 (no os.Exit(...) or panic(...))
• func init() is used too much, eliminate most globals so openstorage can be a library #92 (remove most init functions, remove most global variables)
• strings.Contains(err.Error(), "...") #89 (no reliance on freeform string matching for errors, actually no freeform string matching in general, which leads to api.Filesystem in api/types.go is inconsistently used #71)
• Eliminate api.VolumeID? #76 (dynamic value primitives are not typed)
• api.Filesystem in api/types.go is inconsistently used #71 (static value primitives are not strings)
• Audit for existing casting/nil/panic vulnerabilities #96 (nil/cast checking where no explicit contract on api of external type)
• Remove most uses of private variables in public structs
• Remove most extra variable definitions that are not needed or turn into constants (

  openstorage/api/server/volume.go

  Line 76 in 8d07329

  method := "volumeSet"

  )
• Reduce adding of String() methods on most objects (let the generic `%+v' take care of it)
• Overuse of newlines within methods
• single errors are scoped within an if statement:

// no
err := foo()
if err != nil {
  return nil, err
}
// yes
if err := foo(); err != nil {
  return nil, err
}

• Blank imports should have explanation (

  openstorage/volume/enumerator.go

  Line 6 in 8d07329

  _ "sync"

  )

Other things:

• Code checked in that has warnings https://golang.org/cmd/cgo/ is disallowed
• Code checked in that does not pass make build is disallowed
• Code checked in that does not pass make docker-test is disallowed
• Code always includes relevant vendoring (make docker-test takes care of this basically)
• Code that breaks the build is immediately fixed
• Once completed, go vet, errcheck, and golint fails are considered broken code
• Disallowed = big time scolding and/or rollback

Thoughts?

[bufdev]

@jvinod @gourao

[bufdev]

An example of most of the things here: https://github.com/peter-edge/go-protolog

[gourao]

+1. Also congrats on PR100!

[jvinod]

all proposals LGTM ..will comment on individual requests separately. w.r.t sub directories concatenating parent dir, I'd say no. Each dir should behave like an orphan

[bufdev]

Other things, more for myself to note:

• Actual names for variables, api.Volume is not v, vol, whatever, its volume, a request is not req, createReq, r, its request
• Pointers instead of structs
• function parameters/struct initialization/function calls etc are either on one line or each parameter/field has a new line for it

[bufdev]

File order:

package pkg

const (
  ...
)

var (
 ...
)

// public struct

// public struct functions

// private struct functions

// private functions but only if they just apply to this struct, otherwise in a common file

[bufdev]

Empty structs:

// no
type EmptyStruct {
}
// yes
type EmptyStruct {}

[bufdev]

fa57fcf

[jvinod]

merged