Add support for types which implement encoding.TextUnmarshaler #9
Conversation
|
|
||
| // Check if the struct field type implements the encoding.TextUnmarshaler | ||
| // interface. | ||
| if structField.Type().Implements(reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem()) { |
There was a problem hiding this comment.
I don't believe this code path is currently tested. Moreover, I think it should be required that the TextUnmarshaler interface be implemented with a pointer receiver, otherwise the holder would not be mutated when UnmarshalText was called.
Am I missing something @albrow? I think this code path should be an error.
There was a problem hiding this comment.
Technically, it is not impossible for a type to correctly implement TextUnmarshaler with a non-pointer receiver for the UnmarshalText method. If you have a reference type within the receiver type, it will be settable even if the receiver itself is not. I added a test case to illustrate this:
// customUnmarshallerWrapper also implements the UnmarshalText method by calling
// it on its own *customUnmarshaller.
type customUnmarshallerWrapper struct {
um *customUnmarshaller
}
// UnmarshalText simply calls um.UnmarshalText. Note that here we use a
// non-pointer receiver. It still works because the um field is a pointer. We
// just need to be sure to check if um is nil first.
func (cuw customUnmarshallerWrapper) UnmarshalText(text []byte) error {
if cuw.um == nil {
return nil
}
return cuw.um.UnmarshalText(text)
}It doesn't have to a pointer, either. It could also be a type whose underlying type is a pointer (e.g. type myCustomType *int) or a builtin reference type like map.
Honestly though, after looking at this again, I'm not sure how often it will occur in the real world. I would be open to hearing an argument that this edge case is not necessary and should be removed.
There was a problem hiding this comment.
I see. I hadn't considered this...
While the example you have here with customUnmarshallerWrapper seems a little unlikely to come up, I think think unmarshalling to a builtin reference type is something we would want to support. 👍
envvar/envvar.go
Outdated
| results := structField.Addr().MethodByName("UnmarshalText").Call([]reflect.Value{reflect.ValueOf([]byte(v))}) | ||
| if !results[0].IsNil() { | ||
| err := results[0].Interface().(error) | ||
| return fmt.Errorf("envvar: Error parsing environment variable %s: %s\n%s", name, v, err.Error()) |
There was a problem hiding this comment.
can we add a test for this new error condition as well?
|
@ntindall thanks for taking a look at this :) I also added some test cases for setting default values on a field which implements |
envvar/envvar_test.go
Outdated
| @@ -40,6 +48,9 @@ func TestParse(t *testing.T) { | |||
| FLOAT32: 0.001234, | |||
| FLOAT64: 23.7, | |||
| BOOL: true, | |||
| TIME: time.Date(2017, 10, 31, 14, 18, 0, 0, time.UTC), | |||
| CUSTOM: customUnmarshaler{strings: []string{"foo", "bar", "baz"}}, | |||
There was a problem hiding this comment.
nit: inconsistent formatting with line 92
envvar/envvar_test.go
Outdated
| type customUnmarshalerWrapper [3]string | ||
|
|
||
| // UnmarshalText simply calls um.UnmarshalText. Note that here we use a | ||
| // non-pointer receiver. It still works because the um field is a pointer. We |
There was a problem hiding this comment.
comment seems out of date now, (i think this is a better example than we have in the discussion, though).
There was a problem hiding this comment.
I'm sorry, there were actually some unintended changes included in one of the recent commits. That is why some of the comments don't line up with the code. I agree that the version of customUnmarshalerWrapper which used a slice type ([]string) is easier to understand, but unfortunately it doesn't work. You can't set the value of a slice the way that we would like to.
I've fixed the PR so that now only the intended changes are included, and the comments are re-aligned with the code.
(Technically the built-in slice type contains a pointer to an underlying array. While you can change that array, the changes won't necessarily be reflected in any slices that point to the same array. IMO this idiosyncrasy will lead to more confusion than just using a pointer in our examples and tests).
envvar/envvar_test.go
Outdated
| // an error. | ||
| type alwaysErrorUnmarshaler struct{} | ||
|
|
||
| func (eu alwaysErrorUnmarshaler) UnmarshalText(text []byte) error { |
There was a problem hiding this comment.
Maybe its pedantic, but should we test the case for the pointer receiver as well since that goes down a different code path?
|
|
||
| // Check if the struct field type implements the encoding.TextUnmarshaler | ||
| // interface. | ||
| if structField.Type().Implements(reflect.TypeOf([]encoding.TextUnmarshaler{}).Elem()) { |
There was a problem hiding this comment.
I see. I hadn't considered this...
While the example you have here with customUnmarshallerWrapper seems a little unlikely to come up, I think think unmarshalling to a builtin reference type is something we would want to support. 👍
| } | ||
|
|
||
| // customUnmarshalerWrapper also implements the UnmarshalText method by calling | ||
| // it on its own *customUnmarshaler. |
There was a problem hiding this comment.
comment is out of date now.
|
@ntindall I believe I have addressed all of your comments. Should be ready for final review. |
|
Thanks @ntindall. Looks like I don't have the ability to merge, so I'll leave that to you. |
|
@albrow could you please rebase this against master and fix the conflicts? |
This reverts commit e3cf65d.
The previous commit with the same message contianed some unintended changes. I reverted that commit and created this new one with only the intended changes.
|
@ntindall finished rebasing. My code also uses the new error types now. |
|
LGTM. Thanks @albrow ! |
This allows the go-envvar package to support common types in the standard library such as time.Time. It also allows support for custom types, provided they implement the interface.