probe-lab / go-kademlia

Generic Go Kademlia implementation
Other
17 stars 4 forks source link

Prefer simple config types over function-style options #64

Closed iand closed 11 months ago

iand commented 1 year ago

Function-style options are used in constructors as follows

NewThing(name string, opts ...Option) *Thing

The are very commonly used (especially in IPFS libraries) but they have a number of negative traits:

  1. how to handle duplicate options, which can occur when combining lists of options from multiple sources
  2. how to handle options that have overlapping responsibilities, such as options that touch 2 or more aspects of the object
  3. how to remove an option in a configuration chain, such as when overriding options read from a config file by an option derived from a flag
  4. they are verbose to write since you need to prefix each one with the package name
  5. they clutter the documentation with many extra functions
  6. they are hard to find in documentation unless they are named with a standard pattern or grouped as constructors
  7. it's difficult to inspect the default configuration or log configuration in use

Using a Config struct is simple and avoids the problems above. The constructor signature looks like:

NewThing(name string, cfg *Config) *Thing

Passing nil is interpreted as using the default config. It's conventional to provide a DefaultConfig function that returns a non-nil Config with default values populated. Users can then override fields. It's also easy to inspect the code to determine the default config.

The Config pattern is used in some IPFS packages (go-log) and it's commonly used in the Go standard library (Conn.BeginTx, jpeg.Encode, crypto.Signer) whereas function-style options are never used.

A user of the package can create their own function-style options to build config if they wish.

guillaumemichel commented 11 months ago

Agreed, it makes more sense! I was dumbly copying the same config format as in go-libp2p-kad-dht https://github.com/libp2p/go-libp2p-kad-dht/blob/master/internal/config/config.go

iand commented 11 months ago

Cool. It seems we have consensus.