Support Text unmarshaler in Flatten Mangler #14
Conversation
transform/flatten_mangler.go
Outdated
| if err != nil { | ||
| return out, err | ||
| // only flatten if it doesn't implement TextUnmarshaler | ||
| if !sf.Type.Implements(textMValue) { |
There was a problem hiding this comment.
I think we also need to check reflect.PtrTo(sf.Type).Implements(textMValue), since Implements doesn't check both the concrete type and the pointer-type. (in just about all cases TextUnmarshal() should take a pointer receiver)
There was a problem hiding this comment.
As discussed in slack, it's already a pointer when it gets there so checking to see if the concrete type implements it as well
transform/flatten_mangler.go
Outdated
| @@ -11,6 +12,8 @@ import ( | |||
| // DialsTagName is the name of the dials tag. | |||
| const DialsTagName = "dials" | |||
|
|
|||
| var textMValue = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem() | |||
There was a problem hiding this comment.
Hmm, textMValue is a slightly misleading name. (since it's a type)
Let's call it textMType or textMReflectType, so we don't confuse ourselves.
There was a problem hiding this comment.
Good call, also exposed it since it's used in the flags package as well
There was a problem hiding this comment.
The new name is much better.
I'm not sure I like exporting this here. It's not really supposed to be part of the public interface. It's an internal detail. the TextUnmarshaler type is also used in ptrify, and a couple other subpackages in dials. So far the approach we've been using has been to avoid exposing these reflect.Types by c&ping them into the packages that need them.
If we want to export symbols and share them, I think I'd rather have a separate subpackage expressly for the purpose of tracking these types on which everything else can depend. (probably with unexported package-level variables and functions to get the values so they're actually immutable)
There was a problem hiding this comment.
Ok unexposed them for now and we can have a separate PR if we want to share them
transform/flatten_mangler.go
Outdated
| } | ||
| out = append(out, flattened...) | ||
| fallthrough // don't flatten if it implements TextUnmarshaler |
There was a problem hiding this comment.
I feel like we should either move the body of the default case outside the switch block if we want fall-through.
Added benefit: we can invert the Implements check so the body of the if-statement can have a body that consists entirely of break to break out of the switch-statement.
There was a problem hiding this comment.
Oh forgot about break working in switch statements. It's so much better with the break. Thank you!
transform/flatten_mangler.go
Outdated
| out = append(out, flattened...) | ||
| continue | ||
| // don't flatten if it implements TextUnmarshaler | ||
| if nestedsf.Type.Implements(TextMReflectType) || nestedT.Implements(TextMReflectType) { |
There was a problem hiding this comment.
While it's definitely more efficient to lean on pointerification here, I think I'd rather work backward in this check (and the others like it) by checking reflect.PtrTo(nestedT).Implements in addition to nestedT.Implements so it's clear that we're checking both. (it should be fine either way, since these are specifically struct-types, but I'd like to err on the side of clarity)
transform/flatten_mangler.go
Outdated
| @@ -11,6 +12,8 @@ import ( | |||
| // DialsTagName is the name of the dials tag. | |||
| const DialsTagName = "dials" | |||
|
|
|||
| var textMValue = reflect.TypeOf((*encoding.TextUnmarshaler)(nil)).Elem() | |||
There was a problem hiding this comment.
The new name is much better.
I'm not sure I like exporting this here. It's not really supposed to be part of the public interface. It's an internal detail. the TextUnmarshaler type is also used in ptrify, and a couple other subpackages in dials. So far the approach we've been using has been to avoid exposing these reflect.Types by c&ping them into the packages that need them.
If we want to export symbols and share them, I think I'd rather have a separate subpackage expressly for the purpose of tracking these types on which everything else can depend. (probably with unexported package-level variables and functions to get the values so they're actually immutable)
Co-authored-by: dfinkel <davidf@vimeo.com>
No description provided.