uber-go / guide Goto Github PK
View Code? Open in Web Editor NEWThe Uber Go Style Guide.
License: Apache License 2.0
The Uber Go Style Guide.
License: Apache License 2.0
func foo(bar string) error {
if len(bar) == 0
return errors.New("bar must not be empty")
}
// ...
return nil
}
Go has support for first class functions.
package main
import (
"fmt"
"time"
)
func main() {
addr := "127.0.0.1"
newTimeout := 10 * time.Second
// Options must be provided only if needed.
Connect(addr)
Connect(addr, WithTimeout(newTimeout))
Connect(addr, WithCaching(false))
Connect(
addr,
WithCaching(false),
WithTimeout(newTimeout),
)
}
// Options is DB's options
type Options struct {
timeout time.Duration
caching bool
}
// ApplyOption overrides behavior of Connect.
type ApplyOption func(*Options)
// Option overrides behavior of Connect.
// type Option interface {
// apply(*Options)
// }
// type optionFunc func(*Options)
// func (f optionFunc) apply(o *Options) {
// f(o)
// }
// WithTimeout if you need
func WithTimeout(t time.Duration) ApplyOption {
return func(o *Options) {
o.timeout = t
fmt.Printf("With Timeout: %s\n", t)
}
}
// WithCaching if you need
func WithCaching(cache bool) ApplyOption {
return func(o *Options) {
o.caching = cache
fmt.Printf("With Caching: %t\n", cache)
}
}
const (
defaultTimeout = time.Second
defaultCaching = true
)
// Connect creates a connection.
func Connect(
addr string,
opts ...ApplyOption,
) (*Options, error) {
fmt.Println("---== new Connect ==---")
options := Options{
timeout: defaultTimeout,
caching: defaultCaching,
}
for _, apply := range opts {
apply(&options)
}
return &options, nil
}
The correct link is https://github.com/golang/go/wiki/CommonMistakes, not https://github.com/golang/go/wiki/CodeReviewComments
Semi-related to Channel Size is One or None
We've seen cases of deadlocks/unexpected blocking caused by blocking sends to a channel (example: tchannel-go#528). It's generally much safer to use a select with another case for timeout / ctx.Done()
, or a non-blocking send.
There are some use-cases where it's safe; typically when the number of writers is bound to channel size and can be verified easily, so not sure how generally applicable this is.
Creating this to track adding a general guideline around logging-vs-returning
errors, and doing only one of the two at any error handling site to avoid
double logging.
For struct initialization where some of the fields are zero values, we should establish whether we prefer for those fields to be initialized to zero values explicitly or omitted:
foo := Foo{
Count: 1,
Items: nil,
Default: "",
Admin: true,
}
// or
foo := Foo{
Count: 1,
Admin: true,
}
Opinion: the latter is preferable because it reduces code noise from things that don't matter at this time. You only specify the things that have meaning/value and everything else is the default.
Follow up: When all fields are zero value, do we want to use the explicit struct initialization form or var
?
foo := Foo{}
// or
var foo Foo
Opinion: the latter because it scans differently (visually) and differentiates from Foo{}
where Foo
is a map or slice type (see also, Initializing Maps). It makes it clear that this is something that is probably not going to be used as-is and will be filled separately. ({}
form to initialize structs still applies when not stored in a variable or fields are non-zero: do(Stuff{...})
.)
The suggestion:
"The zero value (a slice declared with var) is usable immediately without make()."
Under "nil is a valid slice" giving example:
// GOOD
var nums []int
if add1 {
nums = append(nums, 1)
}
if add2 {
nums = append(nums, 2)
}
Is not entirely true. In this case it would work only because append creates and returns a new slice. However if you were to do:
var nums []int
nums[0] = 42
It would panic.
So perhaps the wording should somehow clarify this?
This is a controversial topic, so I'll prelude this with: no hard limits.
We should establish a good general limit to aim for as a guidance with the explicit statement that it's okay to go over the limit if the other form is more readable.
Initial proposal: 99 characters, assuming tabs are 4 characters wide. This is the same limit as Rust (see this discussion for why 99, not 100).
(My personal preference is soft limit of 79, but I suspect that'll be a harder sell so I'm starting this discussion with 99.)
import (
"fmt"
"os"
// third party packages
"go.uber.org/atomic"
"golang.org/x/sync/errgroup"
// project packages
"github.com/user/project"
)
I love the uber-go guide! Thanks for sharing your accumulated experience. While I am aware of awesome-go-linters, I wonder if there are any specific recommended linters that compliment this guide.
Alternatively, are there any things in this guide that you wish there was a linter for? We at Khan Academy are contributing to some existing ones and writing a few in house ones as we increase our adoption.
Copying my comment from HN thread:
I find it amusing that the advice in the style guide gives a good example contradicting another good example, and contains a subtle bug.
In the "Reduce Scope of Variables", second good example leaks an open file when WriteString fails, because it doesn't follow the own advice of "Defer to Clean Up" if you are curious.
(Handling that properly with a defer is a bit more tricky - something like https://play.golang.org/p/l1PeWM3Tisg).
Issue title says it all. Thank you for this great write-up :)
Maybe we could follow the new features introduced in Go1.13, see https://blog.golang.org/go1.13-errors
For instance, we can use errors.Is, instead of expose some functions like IsFooError
. The former is more readable and concise.
As it's not specifically called out I am looking to discuss adding our opinion (if any) around function parameters that are not variables or constants.
My motivation for failing this issue and want others' feedback was while doing some simple channel read loop, it occurred to me that we can do a blocking read as a function parameter. My opinion is the difference between <-event
and event
in this use case has a massive impact on behavior.
handleEvent(<-event)
update := <-event
handleEvent(update)
handleEvent(getEvent())
My opinion is that the first case should be avoided. As for the others, I'm not sure I have a strong opinion.
I understand this is specific to a channel in this example, and guidance ideally would not be specific to channels, but I also acknowledge that my point of <-
being the issue is specific to channels.
This is a relatively common Go idiom equivalent to Java's "implements", Rust's "impl ... for", etc.
It
(*Type)(nil)
gives Implement Missing Methods
option).Bad | Good |
---|---|
type A struct{}
func New() A {
return A{}
}
func (a A) Read(p []byte) (n int, err error) {
panic("implement me")
}
func (a A) Close() error {
panic("implement me")
} |
type A struct{}
var _ io.ReadCloser = (*A)(nil)
func New() A {
return A{}
}
func (a A) Read(p []byte) (n int, err error) {
panic("implement me")
}
func (a A) Close() error {
panic("implement me")
} |
I'd be happy to submit a PR.
Creating this to track adding a general guideline around use or misuse
of init()
.
We can formulate the full guideline in this issue, but the first piece
of that can be based on the following general advice:
If it doesn't behave exactly the same every time it's called,
regardless of environment, arguments, directory, etc., then it most
definitely does not belong ininit()
.
I think our guideline should distill that and at least the following
points:
init()
if you caninit()
init()
(command lineExpanding on #21, we should make a distinction for exported and unexported types, and call out advice to avoid bleeding implementation details through APIs when embedding.
Let's keep a changelog to track changes made to the guide over time. The
definition of semantic versioning here is a bit fuzzy so the changelog will
probably be versioned by date.
If you want interface methods to modify the underlying data, you must use a pointer.
which pointer? pointer of object in assignment statement.
Unexported structs that use a mutex to protect fields of the struct may embed the mutex.
What makes unexported types special? ๐
A mutex in a struct is almost always an implementation detail, used to provide concurrency-safety to the capabilities (methods) of the type. Embedding it allows callers to manipulate it directly, but that's an abstraction leak, breaks encapsulation, and is just an enormous maintenance risk, for what I hope are obvious reasons. Whatever callers are doing when they take and release the lock can equally well be expressed as a method, right?
Is there some other use case you have that motivates this exception?
Although we bring this up as a practice in reviews, I don't see it referenced anywhere in our style guide. We likely got this from the Naming package contents section of the Package Names blog post.
We should turn this into a general guideline.
https://peter.bourgon.org/blog/2019/09/11/programming-with-errors.html
Would this be a better way to handle/check custom errors?
This one requires some nuance but generally, for public APIs in libraries,
often when a function reference is expected, an interface is preferable.
Demonstration:
For example, in Zap, we have [zapcore.EncoderConfig] with a few EncodeFoo
fields that all accept a FooEncoder
type, where FooEncoder
is a function
reference.
type EncoderConfig struct {
// ...
EncodeCaller CallerEncoder
// ...
}
type CallerEncoder func(EntryCaller, PrimitiveArrayEncoder)
This is limiting because this means that a CallerEncoder can only ever have
that signature.
If it was, on the other hand, an interface, we would have a non-breaking
upgrade path to change that signature with the help of upcasting.
type CallerEncoder interface {
EncodeCaller(EntryCaller, PrimitiveArrayEncoder)
}
For example, if we decided to change how we represent caller information, we
could do that by declaring a new interface, and specifying that if the
CallerEncoder implements that interface, we'll use it.
type CallFrame struct{ ... } // theoretical new representation
type CallFrameEncoder interface {
EncodeCallFrame(CallFrame, PrimitiveArrayEncoder)
}
type EncoderConfig struct {
// ...
// If this implements CallFrameEncoder, that will be used instead.
EncodeCaller CallerEncoder
}
The above is just a demonstration of a case where this guidance would have
been useful. If we liked this, we would have to turn it into a nuanced
guidance. The guideline could largely be ignored for unexported APIs, and
largely just applies to exported API surfaces.
The style guide is nicely written, and (afaict) unambiguous. However, it can be a bit hard to piece together the entire picture just from reading rules (even with examples). In particular, I found the function grouping and ordering section hard to grok: https://github.com/uber-go/guide/blob/master/style.md#function-grouping-and-ordering
Proposal: let's add a "kitchen sink" example file containing a large subset of the style guide's rules, with comments. I think this would be most helpful to elucidate the "file structure" rules, e.g. ordering of types, variables, methods, etc.
A brief example:
package example
// Consts go at the top (reference here)
const (
)
// Next vars (reference here)
var (
)
// Next type defs
type Foo struct{
}
type Bar struct{
}
func NewFoo() *Foo {
return &Foo{}
}
// Group methods by receiver
func (f *Foo) DoFooThing() {
// maybe throw in some examples r.e. early return etc here.
}
func (b *Bar) DoBarThing() {
}
// Freestanding functions are last. https://github.com/uber-go/guide/blob/master/style.md#function-grouping-and-ordering
func helper()
Uber lame source code from China ewwwwww fuck Uber
With Go 1.13, we should add some guidance for the %w
verb.
2 different objects can have the same address in memory if they are empty structs (0 sized). The Go spec calls this out:
Two distinct zero-size variables may have the same address in memory.
This can still be surprising, and lead to issues when the pointer value is used for comparisons, here's a couple of examples that have some unexpected results.
Add 2 unique elements, but since they have the same address, only a single element is added:
type operation struct{}
var (
add = &operation{}
sub = &operation{}
)
m := map[*operation]int{
add: 1,
sub: 2,
}
fmt.Println(m)
// Output:
// map[0x553f28:2]
Using context keys using pointers to empty structs which end up being the same,
type contextKey struct{}
var (
userID = &contextKey{}
userEmail = &contextKey{}
)
ctx := context.WithValue(context.Background(), userID, "user-id")
ctx = context.WithValue(ctx, userEmail, "[email protected]")
fmt.Println("userID", ctx.Value(userID))
fmt.Println("userEmail", ctx.Value(userEmail))
// Output:
// userID [email protected]
// userEmail [email protected]
I think taking addresses to empty structs (which I think are the only 0-sized objects) should be discouraged. I'd also like to build a linter for this to flag it automatically.
I still have some confusions about writing units. I hope the guide could contains some suggestions of writing tests. Maybe you could consider:
Thank you.
This is to track adding a guideline around use of log.Fatal. Our
recommendation in code reviews has been to have exactly one log.Fatal per
application: in the main. Everything else should return an error.
Channel Size is One or None
Although I definitely appreciate and agree with the intent here, the rule as expressed forbids common patterns, such as the semaphore:
semaphore := make(chan struct{}, 10)
var wg sync.WaitGroup
for i := 0; i < 100; i++ {
wg.Add(1)
go func(i int) {
defer wg.Done()
semaphore <- struct{}{} // acquire
defer func() { <-semaphore }() // release
println(i)
}(i)
}
wg.Wait()
And scatter/gather:
// Scatter
values := []int{1, 2, 3, 4, 5}
c := make(chan int, len(values))
for _, v := range values {
go func(v int) {
c <- process(v)
}(v)
}
// Gather
var sum int
for i := 0; i < cap(c); i++ {
sum += <-c
}
Perhaps "Channels should be unbuffered by default"? There's nothing particularly special about a capacity of 1, I don't think, and all of the provided rationale still applies.
Our recommendation right now includes this pattern,
type Option interface{ apply(*options) }
type optionFunc func(*options)
And then all options are implemented using optionFunc
and closures.
func WithTimeout(d time.Duration) Option {
return optionFunc(func(o *options) {
o.Timeout = d
})
}
This is not the pattern we should be encouraging because it makes Option
s
too opaque. We cannot print them or compare them, or introspect values inside
them.
We should instead recommend declaring a new type for each option.
type timeoutOption time.Duration
func WithTimeout(t time.Duration) Option {
return timeoutOption(t)
}
func (o timeoutOption) apply(opts *options) {
opts.Timeout = time.Duration(o)
}
// And
type loggerOption struct{ L *zap.Logger }
func WithLogger(l *zap.Logger) Option {
return loggerOption{L: l}
}
func (o *loggerOption) apply(opts *options) {
o.Logger = o.L
}
Given the lack of IntelliJ/Goland support for Table Driven Tests (and by support, I mean the ability to run tests individually from within the IDE), I feel like the recommendation for table driven tests feels premature.
Instead of Table Driven Tests, how about something like this:
func TestSplitHostPort_Full(t *testing.T) {
give := "192.0.2.0:8000"
wantHost := "192.0.2.0"
wantPort := "8000"
testSplitHostPort(t, give, wantHost, wantPort)
}
func TestSplitHostPort_NoHost(t *testing.T) {
give := ":8000"
wantHost := ""
wantPort := "8000"
testSplitHostPort(t, give, wantHost, wantPort)
}
func testSplitHostPort(t *testing.T, give string, wantHost string, wantPort string) {
host, port, err := net.SplitHostPort(give)
require.NoError(t, err)
assert.Equal(t, wantHost, host)
assert.Equal(t, wantPort, port)
}
I am more in favor of @davecheney 's constant-errors than using var.
type Error string
const ErrCouldNotOpen = Error("could not open")
is better than
var ErrCouldNotOpen = errors.New("could not open")
In the section Avoid Naked Parameters, this example:
type Status int
const (
StatusReady = iota + 1
StatusDone
// Maybe we will have a StatusInProgress in the future.
)
Shouldn't it be? :
type Status int
const (
StatusReady Status = iota + 1 // Notice `Status` type here
StatusDone
// Maybe we will have a StatusInProgress in the future.
)
very thasks,it is a good inception for me!
Test tables section https://github.com/uber-go/guide/blob/master/style.md#test-tables has this code
for _, tt := range tests {
t.Run(tt.give, func(t *testing.T) {
host, port, err := net.SplitHostPort(tt.give)
require.NoError(t, err)
assert.Equal(t, tt.wantHost, host)
assert.Equal(t, tt.wantPort, port)
})
}
It reused tt
across loops. While it would work in this particular example it may lead to issues when running in parallel because all runs will receive last item of tests
slice.
Possible fix is to create a variable to use in closure.
for _, tt := range tests {
tt := tt
t.Run(tt.give, func(t *testing.T) {
host, port, err := net.SplitHostPort(tt.give)
require.NoError(t, err)
assert.Equal(t, tt.wantHost, host)
assert.Equal(t, tt.wantPort, port)
})
}
See Prefer Specifying Map Capacity Hints
, this link is obsolete
Actually []int{} and nil are not equivalent. Replacement of one to another may cause broken api.
Example:
https://play.golang.org/p/MojffFnM8Gn
There should be warning about that.
This one will require some careful phrasing but the gist of the proposal is:
For unexported types, it's beneficial to export fields or methods to mark them as part of their "public" API is for use by other unexported types.
As a contrived example,
// counter.go
type countingWriter struct {
writer io.Writer
count int
}
func (cw *countingWriter) Write(b []byte) (int, error) {
n, err := cw.writer.Write(b)
cw.inc(n)
return n, err
}
func (cw *countingWriter) inc(n int) {
cw.count += n
}
// another.go
func foo(w io.Writer) error {
cw := &countingWriter{writer: w}
writeABunch(&cw)
fmt.Println("wrote", cw.count, "bytes")
}
It would be useful for the author of countingWriter
to indicate which fields they intended to be accessed directly, and which are "private". The example above could be:
type countingWriter struct {
Writer io.Writer
Count int
}
func (*countingWriter) inc(int)
This indicates that int
is for private use but the Writer and Count fields may be accessed freely. If the author wanted to protect writes to Count, maybe they'd switch to:
type countingWriter struct {
Writer io.Writer
count int
}
func (*countingWriter) Count() int
Obviously, there's no enforcement and this only works as a general guidance to indicate "publicness" of a field/method of a private type, but as a reader/consumer of a private type in a large package, one might find value in knowing what they should or should not touch.
Hi all
The Korean version of uber-go-style-guide has not been updated for 9 months.
There have been many updates to the current English version of the uber-go-style guide,
and I think the Korean translation does not fully reflect the current version.
If you don't mind, I wonder if I can translate the latest version of uber-go-style-guide and share it on Github.
- Wrapped errors using "pkg/errors".Wrap
Since Go 1.13, wrapping errors is canonically done with fmt.Errorf and %w.
Relatedly, in the Error wrapping section,
return fmt.Errorf("new store: %v", err)
should probably use %w instead of %v โ or, if %v was intentional, should describe when to use %v vs. %w.
โ
Do the clients need to detect and handle this error? If so, you should use a custom type, and implement the Error() method.
Custom types are appropriate only when consumers need extra information about the error beyond it's identity. If consumers only need to handle the error by identity, then the more appropriate choice is a sentinel error, i.e. var ErrFoo = errors.New("...")
. ref
โ
Be careful with exporting custom error types directly since they become part of the public API of the package. It is preferable to expose matcher functions to check the error instead.
It's true that both exported sentinel errors and error types become part of your package API, but the same is true of the matcher function suggested here, and they both have ~equivalent impact on API compatibility. What's more, the consequent advice to return unexported error types e.g.
type errNotFound struct {
file string
}
is surely incorrect, because the extra information captured in the type is inaccessible to callers. (In fact, it's almost always an error to return an unexported type from an exported function or method.)
for _, o := range opts { o(&options) }
should be
for _, o := range opts { o.apply(&options) }
We should discourage fields or variables named after built-in types:
var string string
type Foo struct{ error error }
As variable names, they shadow the actual type, making it impossible to use that type in that scope again:
var string string
var x string // error: string is not a type
Carrying this over to field names is just a matter of consistency in style.
a nil slice is a valid slice and there's no problem doing append(nilSlice, elem)
. However, a nil map isn't and it has to be instantiated before further use.
How do you recommend to instantiate an empty map? m := map[type]type{}
or m := make(map[type]type)
or something else?
Related to #66, we should add guidance for error handling, propagation, and continuity through code (either all the way to main()
, or to a library's outermost API).
I challenge the rational behind this recommendation.
_ prefix:
// foo.go
const (
defaultFooPort = 8080
defaultFooUser = "user"
)
// bar.go
func Bar() {
defaultBarPort := 9090
...
}
or simply
// foo.go
const (
defaultPort = 8080
defaultUser = "user"
)
// bar.go
func Bar() {
port := 9090
...
}
Based on the above I believe prefixing variables with _ should be considered not just unnecessary but harmful.
In the case of:
data := []byte("Hello world")
for i := 0; i < b.N; i++ {
w.Write(data)
}
Why should you not using bytes.Repeat instead of this snippet of code ?
The intent of this code is exactly the same as bytes.Repeat without the benefits.
I have provided a Chinese translation and I hope it can be included.
One more comma
newDay:= t.AddDate(0 /* years */, 0, /* months */, 1 /* days */)
shoud be fixed:
newDay:= t.AddDate(0 /* years */, 0 /* months */, 1 /* days */)
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.