Skip to content
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

Merged

Conversation

@RussellLuo
Copy link
Contributor

@RussellLuo RussellLuo commented Aug 31, 2020

This fixed #156.

I know the existence of #199, but this PR supports:

  • Correcty squashing embedded struct pointers while decoding map from struct
  • As well as squashing embedded struct pointers while decoding struct from map
@SimonPrague
Copy link

@SimonPrague SimonPrague commented Nov 11, 2020

Any update on this? I'd love to take advantage of this feature. Thanks for the update.

@mitchellh
Copy link
Owner

@mitchellh mitchellh commented Nov 26, 2020

Thank you!

@mitchellh mitchellh merged commit d5b1da7 into mitchellh:master Nov 26, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RussellLuo RussellLuo deleted the RussellLuo:add-support-for-squashing-struct-ptr branch Nov 27, 2020
"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": "",
},
Comment on lines +2392 to +2411

This comment has been minimized.

@dnephin

dnephin Jan 12, 2021
Contributor

I think this test change was masking the bug described in #231

This PR should not have changed any tests with omitempty, right?

This comment has been minimized.

@mitchellh

mitchellh Jan 12, 2021
Owner

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()
}
Comment on lines +865 to +868

This comment has been minimized.

@dnephin

dnephin Jan 12, 2021
Contributor

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.

This comment has been minimized.

@mitchellh

mitchellh Jan 12, 2021
Owner

I got it to pass, one sec.

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants