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
Add support for squashing embedded struct pointers #205
Add support for squashing embedded struct pointers #205
Conversation
Any update on this? I'd love to take advantage of this feature. Thanks for the update. |
Thank you! |
"omittable-nested": map[string]interface{}{ | ||
"Vbar": map[string]interface{}{ | ||
"Vbool": false, | ||
"Vdata": interface{}(nil), | ||
"Vextra": "", | ||
"Vfloat": float64(0), | ||
"Vint": 0, | ||
"Vint16": int16(0), | ||
"Vint32": int32(0), | ||
"Vint64": int64(0), | ||
"Vint8": int8(0), | ||
"VjsonFloat": float64(0), | ||
"VjsonInt": 0, | ||
"VjsonNumber": json.Number(""), | ||
"VjsonUint": uint(0), | ||
"Vstring": "", | ||
"Vuint": uint(0), | ||
}, | ||
"Vfoo": "", | ||
}, |
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.
I think this test change was masking the bug described in #231
This PR should not have changed any tests with omitempty, right?
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.
Yes you're probably right. I thought this would be safe because this only added tests but I see this is a change.
if v.Kind() == reflect.Ptr && v.Elem().Kind() == reflect.Struct { | ||
// Handle embedded struct pointers as embedded structs. | ||
v = v.Elem() | ||
} |
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.
I think this is what is causing the bug in #231
If I remove this line my tests pass, but of course the new tested added in this PR fail.
I attempted to move this check to lower down, but I have not got it working yet. It seems like we should not be changing v
, but we may need to deref the ptr in a few places to make this work.
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.
I got it to pass, one sec.
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.
This fixed #156.
I know the existence of #199, but this PR supports: