WIP custom value interface-based proposal #2
Conversation
| SaveInt64To(dest *int64) error | ||
| } | ||
|
|
||
| type CustomValue interface { | ||
| json.Unmarshaler |
There was a problem hiding this comment.
This is somewhat problematic - it will force users to implement the json.Unmarshaler interface for custom values, even if they don't use JSON for configuring their application (e.g. they only rely on env vars and/or CLI flags).
That's why I went with encoding.TextUnmarshaler when I started experimenting with custom types - it is the more generic interface. Basically, if a value can be parsed from a textual form, it should be able to implement TextUnmarshaler. And, by definition, CLI flags and environment variables are text, while a lot of custom variables in JSON are also represented as strings. I assume that's why the default json.Unmarshal() behavior also supports TextUnmarshaler as a fallback (emphasis mine):
To unmarshal JSON into a value implementing the Unmarshaler interface, Unmarshal calls that value's UnmarshalJSON method, including when the input is a JSON null. Otherwise, if the value implements encoding.TextUnmarshaler and the input is a JSON quoted string, Unmarshal calls that value's UnmarshalText method with the unquoted form of the string.
So, requiring custom types to implement TextUnmarshaler seems much less onerous than requiring them to implement json.Unmarshaler 🤷♂️ The vast majority of custom types should be fine, and the few that aren't should be able to implement the croconf.Field interface directly, I think...
The problem is how do we "upgrade" to json.Unmarshaler for the types that support it, and the best way I can think of right now is to just type assert it 😞 That is, adopt an approach reminiscent of progressive enhancement - require custom types to at least implement TextUnmarshaler, but the JSON source will check if they also implement json.Unmarshaler and use it if they do. What do you think? cc @codebien
There was a problem hiding this comment.
Personally, I like the idea to use interfaces because it makes the extension of the API more idiomatic, following something already used by developers with the encoding package.
I agree to reduce the dependency from the specific implementations but I don't like to require having the interface check directly in the code implementation that it would potentially open to a good number of bugs. But I haven't a concrete better alternative.
| } | ||
|
|
||
| func (dcv defaultCustomValue) SaveCustomValueTo(dest CustomValue) error { | ||
| dest = dcv.value |
There was a problem hiding this comment.
I don't think this will work, since I think dest won't be treated as a pointer here 😕 That's why json.Unmarshaler and encoding.TextUnmarshaler are implemented with pointer receivers, so they have an internal pointer to the value they're supposed to "fill" with the data they're unmarshaling.
| @@ -80,3 +81,14 @@ func (d *Duration) UnmarshalJSON(data []byte) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // ParseFromString implements croconf.CustomValue interface | |||
| func (d *Duration) ParseFromString(data string) error { | |||
There was a problem hiding this comment.
I think we can move to UnmarshalText
| SaveInt64To(dest *int64) error | ||
| } | ||
|
|
||
| type CustomValue interface { | ||
| json.Unmarshaler |
There was a problem hiding this comment.
Personally, I like the idea to use interfaces because it makes the extension of the API more idiomatic, following something already used by developers with the encoding package.
I agree to reduce the dependency from the specific implementations but I don't like to require having the interface check directly in the code implementation that it would potentially open to a good number of bugs. But I haven't a concrete better alternative.
artych
changed the title
No description provided.