Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Stop growing Config object #1548

methane started this conversation in Ideas
Discussion options

Current Config object have several issues.

  1. User must not use zero-value. They should use NewConfig() instead.
  2. New and old fields are coexists: tls and tlsConfig.
  3. We have exposed Params map[string]string that makes fixing Is it possible to execute set variable sequentially? #1455 difficult.

The second problem will growth forever. We may add Compression bool and CompressionMethod []str later. To stop making Config bigger and more complex, I am considering two options:

Idea 1: Add methods to *Config

This is simpler approach. Keep existing Config fields but stop adding public fields anymore.
We will add methods to Config instead:

  1. func (c *Config) SetCompression(enable bool)
  2. func (c *Config) SetTimeTruncate(d time.Duration) (revert TimeTruncate field added recently but never released yet)

Example:

cfg := mysql.ParseDSN(dsn)
cfg.SetCompression(true)
cfg.SetTimeTruncate(time.Millisecond)

Idea 2: Use "Functional Option Pattern"

This is similar to the previous, but add one func (c *Config) SetOptions(option...) method instead of many Set***() methods and have many FOP functions like:

  1. func Compression(enable bool) option
  2. func TimeTruncate(d time.Duration) option

Example:

cfg := mysql.ParseDSN(dsn)
cfg.SetOptions(
 Compression(true),
 TimeTruncate(time.Millisecond),
)

Idea 1 is simpler. But with Idea 2, we can reuse option functions in new API and stop using Config completely like this:

// in the future...
connector, err := mysql.NewConnectorOptions(
 User("root"), Password("secret"),
 Host("mydb.local"),
 Database("myappdb"),
 Compression(true),
 TimeTruncate(time.Millisecond),
)

How do you think? @shogo82148, @arnehormann

You must be logged in to vote

Replies: 2 comments

Comment options

+1 for Idea 2.

Adding a method like SetCompression is virtually no different than adding a field.

You must be logged in to vote
0 replies
Comment options

One nit: I'd love to have something that maps to the URLs from https://gocloud.dev/howto/sql/ - but that's probably a lost case anyway.

For a migration away from Config, idea 2 is definitely better. It could also support a solution to Problem 3 with e.g. OptionSequence(...Option) wrapping other options that have to be applied in order.
Checking order and consistency of the resulting configuration might be a headache (e.g. SetCompression(true) and SetCompression(false) in one call), but that's possible with the err-return.

Sounds good to me - only the mysql. prefixes for callers of the options might be mildly annoying.

I could think of one other way to handle this - that cuts down the mysql package statements:
provide a new ConfigOptions struct with the funcs from idea 1 and an additional ApplyTo(Config) error to apply it to either the old Config struct or a new alternative. We could use generics and/or a type assertion to restrict the argument to those types. It's just minor savings, though, probably not worth the effort.

You must be logged in to vote
0 replies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Category
Ideas
Labels
None yet

AltStyle によって変換されたページ (->オリジナル) /