uber-go / guide Goto Github PK

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.

TangoEnSkai/uber-go-style-guide-kr#8

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.

func foo(bar string) error {
if len(bar) == 0
return errors.New("bar must not be empty")
}
// ...
return nil
}

One more comma
newDay:= t.AddDate(0 /* years */, 0, /* months */, 1 /* days */)
shoud be fixed:
newDay:= t.AddDate(0 /* years */, 0 /* months */, 1 /* days */)

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.)

Expanding 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.

cc @abhinav @prashantv

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.
)

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?

very thasks,it is a good inception for me!

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:

1. how to define cases?
2. should we define both "pass" and "not pass" cases? or just "pass" cases?
3. how to test errors (or fatals/panics, if needed)?
4. (maybe difficult) some guides about mock or noop servers/clients.

Thank you.

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.

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.

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.

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 in init().

I think our guideline should distill that and at least the following
points:

• Avoid init() if you can
• Pre-computation is okay as long as it's deterministic
• Don't do any form of IO from init()
• Don't rely on mutable global state in init() (command line
  arguments, environment variables, current directory, etc.)

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).

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)
  })
}

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

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.

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.

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.

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")

Go has support for first class functions.

Run at Go Playground

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
}

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)
}

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.

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.

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.

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?

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.

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?

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 Options
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
}

The correct link is https://github.com/golang/go/wiki/CommonMistakes, not https://github.com/golang/go/wiki/CodeReviewComments

I challenge the rational behind this recommendation.

_ prefix:

• makes naming rules confusing & opens up a possibility for an abuse.
  My observation is that people start to use _ prefix left and right
• is redundant & can always be avoided, e.g. Prefix Unexported Globals with _ example can be replaced with

// 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
  ...
}

• package-level variables can be shown as such/highlighted by the IDE if that's your preference (e.g. in GoLand, go to Settings -> Editor -> Code Scheme -> Go -> Variable -> Package local variable).
• feels like python-ism / Hungarian notion in disguise
• generally inconsistent with the rest of Go community

Based on the above I believe prefixing variables with _ should be considered not just unnecessary but harmful.

• 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.)

I have provided a Chinese translation and I hope it can be included.

uber_go_guide_cn

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.

See Prefer Specifying Map Capacity Hints , this link is obsolete

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).

With Go 1.13, we should add some guidance for the %w verb.

If you want interface methods to modify the underlying data, you must use a pointer.

which pointer? pointer of object in assignment statement.

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{...}).)

for _, o := range opts { o(&options) }

should be

for _, o := range opts { o.apply(&options) }

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.

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.

https://peter.bourgon.org/blog/2019/09/11/programming-with-errors.html

Would this be a better way to handle/check custom errors?

This is a relatively common Go idiom equivalent to Java's "implements", Rust's "impl ... for", etc.
It

• asserts that types implement expected set of interfaces (declaration of intent), guarding against breakage caused by method signature changes
• improves readability by making things explicit & acting as an additional form of documentation
• helps with code navigation/generation (e.g. in GoLand Alt+Insert on (*Type)(nil) gives Implement Missing Methods option).

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.