Comment by bbatha | Hacker News Reader

Comment by bbatha | Hacker News Reader

Parent Full thread

This is generally great advice, and aside from the package manager recommendations is still relevant today. I do take issue with a few things though.

1. Don't put your internal libraries in /pkg. /pkg has special GOPATH meaning as "compilation cache". Its not an actual name conflict, but why bother risking it.

2. This is just wrong:

> fmt.Printf is self-contained and doesn’t affect or depend on global state; in functional terms, it has something like referential transparency. So it is not a dependency. Obviously, f.Bar is a dependency. And, interestingly, log.Printf acts on a package-global logger object, it’s just obscured behind the free function Printf. So it, too, is a dependency.

stdout (and the buffer, and mutex on it) are exactly the same as the global log object. In fact, `log.Printf` is more or less just an alias to `fmt.Printf`[0]

3. I wish it had mentioned the functional options pattern in the part about constructors[1]

0: https://github.com/golang/go/blob/b77aad089176ecab971d3a72f0...

1: https://dave.cheney.net/2014/10/17/functional-options-for-fr...

> the functional options pattern

Functional options are controversial in the Go community. I don't like them personally. The idea of having a bunch of functions that exist only to mutate internal state on init is... an odd choice. Google code is riddled with this sort of style.

It's annoying to write client code for another reason: It's hard to discover. For the "term" example in the article, you can't sit in your editor/IDE and type "term." and get a list of setter suggestions, since everything in a single namespace. "term.Speed()" does not sound like an option to me. Function names are generally verbs, and having one called Speed() doesn't read well. And it causes issues if you want to have a type called Speed. Then it has to be term.WithSpeed() or term.SetSpeed() or something. Neither of which is really self-explanatory.

The fact that options are functions has other downsides. The moment you pack a option into a function, they've lost their ability to be introspected. You can't dump the options to a debug log before you invoke the constructor function. You can never ask an option function what it contains.

I prefer representing options using actual data:

  type (
    Option interface {
      isOption()
    }

    Speed int
    RawMode bool
    SomeOtherSetting struct {
      A, B int
    }
  )

Unfortunately, due to how Go interfaces work, you'll have to provide a dummy private method to tie the types together, but it's a small loss. (Go itself uses this pattern in a bunch of places, such as the compiler's AST.)

My biggest object to the functional options argument is the step where

    NewServer(..., Config{})

is considered bad for some reason. As far as I can tell, the entire purported utility of the thing is based on avoiding this obvious approach.

A big config struct is great, but it comes with a big downside: You have to rely on Go's zero value semantics to provide defaults. It's not always clear that 0 or an empty string means "no value".

It means that sometimes you'll have to use dummy values (-1 for "no value", for example) or pointers (nil means optional). It means that any boolean has to be phrased in such a way that false is the default (so if it feels natural to call the option UseKeychain and it should default to true, you have to invert it and call it DontUseKeychain).

Config structs also means it's easier for a caller to construct invalid configs:

  type Config struct {
    // Either stream or channel can be set, but not both
    OutputStream io.Writer
    OutputChan chan Event
  }
  // ...
  pipelines.New(pipelines.Config{
    OutputStream: myFile,
    OutputChan: myChan,
  })

The semantics cannot be guaranteed by the type system, so won't be caught until runtime. Neither will this, of course:

  // ...
  pipelines.New(
    pipelines.OutputStream(myFile),
    pipelines.OutputChan(myChan))

...but in this case we can quite sensibly mandate that the following option overrides the previous one, flipping the internal switch over to the setting you specify. With the Config struct, there's no way for pipelines.New() to know which one should have precedence.

(In this particular case, you can work around it by having a single Output field that uses an interface, but there are other situations where mutually incompatible fields can occur.)

Yep! It was probably overstepping on my part to say that the entire benefit depends on that argument. I should instead have said that the tradeoff does not seem good to me.

It depends on the purpose. Config structs are indeed simpler and make tons of sense for cases where you control all the related code. So yeah, for code where you can do a mostly-automated refactor if you need to change the API, I totally agree that Config structs make more sense and functional options are huge overkill.

For stuff that needs finer-grained control, or long-lived API stability, functional options give you a lot more flexibility.

> Functional options are controversial in the Go community. I don't like them personally. The idea of having a bunch of functions that exist only to mutate internal state on init is... an odd choice. Google code is riddled with this sort of style.

I come from a background in functional programming, and having used functional options in Go for a while now, I completely agree. I've never seen an implementation of them that felt natural or idiomatic to use. They're incredibly awkward, and I basically always prefer the alternative approaches to the functional options style.

Could you give a more complete example of how that works? I had a look at the `go/ast` package but couldn't find anything.

It's in ast.go (https://github.com/golang/go/blob/master/src/go/ast/ast.go). Look how Node, Expr etc. are interfaces, with concrete struct implementations. The interfaces don't do much, only provide position information.

> 1. Don't put your internal libraries in /pkg. /pkg has special GOPATH meaning as "compilation cache". Its not an actual name conflict, but why bother risking it.

It's a little too late for this because it's pretty much an unofficial standard for library code :-) Just look at Kubernetes, Docker or many other major Go-based applications/tools.

Last I checked, pkg is going away as a compilation cache. As of Go 1.10, the compilation is in fact now stored elsewhere: https://golang.org/doc/go1.10

I think `fmt.Printf` is a typo. Immediately preceding this is a code sample referencing `fmt.Fprintf` (which is correct).

Reply on news.ycombinator.com