Repository navigation
Call UnmarshalJSON for embedded types that implement json.Unmarshaler; Update Network v2 Provider Extension to be consistent with other extensions - #304
Conversation
|
cc @jtopjian I needed to make a lot of the same changes you made to get the tests to pass, so I just rolled your changes into here. |
| // Specifies whether the network resource can be accessed by any tenant or not. | ||
| Shared bool `json:"shared"` | ||
|
|
||
| type NetworkExt struct { |
There was a problem hiding this comment.
I think extension names should be renamed to something more unique. Out of curiosity, I tried to mix two extensions in like so (renaming NetworkProviderExt to NetworkExt):
type NetworkWithExt {
networks.Network
provider.NetworkExt
external.NetworkExt
}And got a Go error of:
duplicate field NetworkExt
There was a problem hiding this comment.
Good point. Will change.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
I think I'm able to follow along what's going on here. The only thing I'm unsure about is why a Marshal/Unmarshal is being performed here and again on each anonymous struct?
There was a problem hiding this comment.
I'll add some comments and unit tests. Basically, the first json.Unmarshal will handle any response objects the don't have embedded structs. The other is specifically for embedded structs.
When a struct is embedded, the parent will inherit a method defined on the embedded struct unless that same method is defined on the parent.
|
I'm going to think about this some more and do some more testing: what's there was kind of thrown together in haste (read: I don't like it). In my head, if each embedded struct implements |
|
OK, I'm convinced that it doesn't work like I thought it did. It looks like the following rules are applied for
Case 3 above is the problem we're seeing. Though I'd like for it to work like I thought it should, the current behavior seems consistent. All the above basically means the logic currently in this PR may be necessary to achieve what we're after. |
|
@jrperritt Any reason to keep this one open? |
|
I don't think so. Closing in favor of #443 |
3 participants
For #300 and part of #298