Notes on Go style
kjk opened this issue · 2 comments
Looks like a great project! We need more embeddable storage engines for Go.
- Just want to point out that the way code is structured is Java-like. Idiomatic Go code doesn't nest code (i.e. no
src/devt.de).
The idea is to have import paths as short as possible i.e. instead of import "github.com/kroti/eliasdb/src/dev.de/eliasdb" I should just be able to import "github.com/kroti/eliasdb".
If you consider embeddable library to be main artifact, then github.com/kroti/eilasdb would be an import path for the library.
Executables that are part of the repo and use the library would go into cmd directory (e.g. the server would go into cmd/server).
You can see this structure in both https://github.com/boltdb/bolt and https://github.com/golang/leveldb.
The code can be divided into sub-packages, but from the perspective of API design it would be best if code using the library could do most of the work with just API exposed by top-level API.
In general idiomatic Go code favors "fatter" packages than idiomatic Java code. You can see that boltdb code doesn't even have sub-packages even though it could be logically divided into smaller parts. Another example is https://github.com/cznic/ql (it's embeddable sql database which pretty much uses only top-level package for all the code).
See also https://blog.golang.org/organizing-go-code
-
relative imports are frown upon in Go i.e. instead of
import "devt.de/eliasdb/storage"it should be fully qualified `import "github.com/krotik/eliasdb/devt.de/eliasdb/storage". Using "devt.de/..." only works if you change GOPATH which breaks standard Go workflow i.e. the package is not usably for 99% of Go programmers. This is explicitly mentioned as a thing not to do in https://blog.golang.org/organizing-go-code -
Idiomatic Go code discourages "stuttering" i.e. repeating package name in names of exported functions, types etc. in that package.
For example in package graphstorage, type GraphStorage interface repeats "graph", so it would be type Storage interface. See, for example, how in https://github.com/golang/leveldb/blob/master/record/record.go there is type Reader and not type RecordReader, because even though Reader would be ambiguous globally, it's not ambiguous when scoped to reader package and in Go everything is fully qualified.
Hi Chris,
thanks for your comment.
To point 1)
The reason the Github repository looks like this is that it includes documentation, design documents and tutorials. The actual code repository for embedding of EliasDB doesn't live on Github at all but rather on my own server which is reachable under devt.de. Having an own build server gives me the ability to have regular overnight builds and unit tests.
What you see on Github is a complete Go project structure. People doing a checkout from Github will receive everything while people who do a "go get" will only receive the source code.
I describe these two methods under "Building EliasDB" but maybe it is not clear enough? I do know that this approach is not what other projects do but it does work for me so far and I don't see any drawback in the moment.
I would say the main artifact of EliasDB is the single executable which lives directly under devt.de/eliasdb. The main file gives you a good starting point of how everything fits together.
Architecturally, the closest thing to a top-level API in EliasDB is I think the graph.Manager class. However, there is nothing stopping you from just using the hash and storage API and basically use EliasDB as a key-value store. I am in two minds here, I think I favor slightly the current organisation where you have separate layers each with defined interfaces and the graph package is just one of them.
In "Organizing Go code" (https://blog.golang.org/organizing-go-code) it is said that there is "no hard and fast rule" for package size. I am not a big fan of "fat" packages as it makes reading code harder, in my opinion. The current packages are self contained modules and the unit test runtime for most of them is reasonable - if you change any code you can run the package unit tests frequently without getting annoyed.
To point 2)
The code does only use fully qualified imports. The code "lives" indeed under devt.de and not github.com.
To point 3)
Stuttering is indeed bad practice and I do agree graphstorage.Graphstorage is a stutter. I'll change it.
To avoid stuttering I run golint as part of my build process. In fact my build breaks if I have any golint or go vet messages. In case of graphstorage.Graphstorage the tools seem to accept it.
I've changed now graphstorage.GraphStorage to graphstorage.Storage. I also adjusted the "Building EliasDB" section a bit and explicitly say now when to use which approach for code retrieval.