contrib: simplify constructors options and deprecate OptionFn

[dolmen]

Many packages under contrib/ expose constructors that use the functional options pattern. Each package expose an Option type as well as an OptionFn type. However OptionFn is just an implementation detail that should never have been exposed.

This is a proposal to refactor the implementation of those Option types in a source compatible way that will be transparent to users. This refactor deprecates the OptionFn type (which no user should have ben using because Option is the abstract type) to go towards its removal in the future.

This refactor also allows to use a lighter implementation of the functional options pattern that hides implementation details via private types and type aliases, and uses only concrete types avoiding applying options through a (private) interface.

Implementation

Before:

type Option interface {
	apply(*config)
}

// OptionFn represents options applicable to the constructor.
type OptionFn func(*config)

func (fn OptionFn) apply(cfg *config) {
	fn(cfg)
}

func WithService(name string) OptionFn {
	return func(c *config) {
		c.serviceName = name
	}
}

After:

// OptionFn represents options applicable to the constructor.
type Option = option

// OptionFn was an implementation detail related to [Option].
//
// Deprecated: use [Option].
type OptionFn = option

type option func(c *config)

func (fn option) apply(cfg *config) {
	fn(cfg)
}

func WithService(name string) Option {
	return func(c *config) {
		c.serviceName = name
	}
}

This could be refactored further by changing how options are applied in the contructor by completely bypassing the apply(*config) method:

Before:

func New(opts ...Option) T {
	var cfg config
	defaults(&cfg)
	for _, o := range opts {
		o.apply(&conf)
	}
	// ...
}

After:

func New(opts ...Option) T {
	var cfg config
	defaults(&cfg)
	for _, o := range opts {
		o(&conf)
	}
	// ...
}

[dolmen]

Note: the proposed solution currently raises a golangci-lint error (golangci-lint 2.1.6): unexported-return: exported func WithAnalytics returns unexported type gin.Option, which can be annoying to use (revive)

However this is a false positive that I've reported in mgechev/revive#1406 and that already has a proposed fix in mgechev/revive#1407.

[darccio]

@dolmen Did you validate your proposal against contrib/net/http? The design is to avoid duplicating functional options like we did in v1.

Example:

// v1
func WithStatusCheck(fn func(statusCode int) bool) Option {
    // ...
}

func RTWithStatusCheck(fn func(statusCode int) bool) RoundTripperOption
    // ...
}

This was needed to be able to instrument http.RoundTripper and http.Handler. As they are different types, we had different functions (WrapHandler and WrapRoundTripper).

In v2 these duplicated functional options became one:

// v2
func WithStatusCheck(fn func(statusCode int) bool) OptionFn {
    // ..
}

Here you have an example where WithStatusCheck is used in WrapRoundTripper.

[dolmen]

I haven't yet had a look at contrib/net/http, but I'll do today.

Anyway, as each contrib package is independent my pattern can be applied to just all packages for which it is possible, and I'll provide PR for each one independently. The first one I have submitted is contrib/gin-gonic/gin in #3650.

[darccio]

@dolmen Thanks for your insights. Although we agree that the pattern is applicable to multiple contribs independently, we prefer to have a coherent design across the 50+ contribs in our library.

Provided that the amount of contribs and their complexity are growing, if our current design serves these cases were functions may have to cover multiple types in any given contrib, why should we revert it in some cases that are simpler? What's the issue that are we trying to solve here?

All types exported in this design were done on purpose, as we didn't want our API to return unexported types.

[darccio]

@dolmen Did you have the opportunity to validate your proposal against the specified package? We'd love to hear your feedback.

[darccio]

Closing the discussion for lack of feedback.