-
Notifications
You must be signed in to change notification settings - Fork 2.3k
-
Current Config object have several issues.
- User must not use zero-value. They should use NewConfig() instead.
- New and old fields are coexists: tls and tlsConfig.
- 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:
func (c *Config) SetCompression(enable bool)
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:
func Compression(enable bool) option
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
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 2 comments
-
+1 for Idea 2.
Adding a method like SetCompression
is virtually no different than adding a field.
Beta Was this translation helpful? Give feedback.
All reactions
-
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.
Beta Was this translation helpful? Give feedback.