-
Notifications
You must be signed in to change notification settings - Fork 839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
executor: adds support for anonymous contained structs #371
base: master
Are you sure you want to change the base?
Conversation
HI, wondering if there is any chance to merge this? |
@chris-ramon @pavelnikolov Same question, would you please merge it? |
Same here |
Thanks for letting us know how important this PR are for you guys. I'll jump back to get this merge as soon as I get the chance today/tomorrow. |
d376817
to
d35d558
Compare
These should behave similarly to the equivalent case in https://golang.org/pkg/encoding/json/
defaultResolveStruct from DefaultResolveFn
I think this will resolve the issue without using user-provided resolve function. if typeField.Anonymous && typeField.Type.Kind() == reflect.Struct {
anonymousResolvedValue, err := defaultResolveStruct(valueField, fieldName)
if err != nil {
return nil, err
} else if anonymousResolvedValue != nil {
return anonymousResolvedValue, nil
}
} |
Just commented on the issue, but just saw this PR. I'm still interested in this! |
return valueField.Interface(), nil | ||
} | ||
if typeField.Anonymous && typeField.Type.Kind() == reflect.Struct { | ||
return defaultResolveStruct(valueField, fieldName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bug here. We return too early if an anonymous struct comes before a field we're looking for.
I think what we want to do is run through all the top-level fields first, then dive if the field wasn't found.
Repeat for all anonymous structs until it's found. If not found, then return nil, nil
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a fix, sans a proper test:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @abraithwaite
Any news on this ? |
Any updates on this PR? |
Still waiting for the merge as qell... |
This PR has conflicts. |
Is this still being worked on? |
Not that I am aware of |
Hi |
Bump |
Overview
Test plan
go test ./...