-
-
Notifications
You must be signed in to change notification settings - Fork 271
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 Struct in Map #105
Conversation
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.
@svyotov thanks for the PR. It would be better to add a test that validates that behaviour.
Also, I checked-out that PR and ran the tests, and I think it's breaking current behaviour
--- FAIL: TestMapsWithOverwrite (0.00s)
mergo_test.go:372: Test failed:
got :
map[string]mergo.simpleTest{"d":mergo.simpleTest{Value:61}, "a":mergo.simpleTest{Value:0}, "b":mergo.simpleTest
{Value:42}, "c":mergo.simpleTest{Value:13}}
want :
map[string]mergo.simpleTest{"a":mergo.simpleTest{Value:16}, "b":mergo.simpleTest{Value:0}, "c":mergo.simpleTest
{Value:12}, "d":mergo.simpleTest{Value:61}, "e":mergo.simpleTest{Value:14}}
=== RUN TestMaps
--- FAIL: TestMaps (0.00s)
mergo_test.go:402: Test failed:
got :
map[string]mergo.simpleTest{"a":mergo.simpleTest{Value:0}, "b":mergo.simpleTest{Value:42}, "c":mergo.simpleTest
{Value:13}, "d":mergo.simpleTest{Value:61}}
want :
map[string]mergo.simpleTest{"b":mergo.simpleTest{Value:42}, "c":mergo.simpleTest{Value:13}, "d":mergo.simpleTes
t{Value:61}, "e":mergo.simpleTest{Value:14}, "a":mergo.simpleTest{Value:0}}
@vdemeester I have added a unit test which hopefully is correct and better shows the issue, however I have not managed to make the code work yet. |
@vdemeester If you have pointers regarding how to fix this/ improve this I would love to hear from you and see if I can make it into a new PR. |
Any thoughts on this PR @vdemeester, @imdario? |
I'll check it this evening. |
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.
unsafe
usage worries me a bit, I wonder if it impacts performances. We should create some benchmark tests to make sure we don't regress too much.- It adds some complexity to the code (that seems to be required… but stiff… 😅)
The test is actually green. I'll fix the CI in a separate PR so that we can trust the GIthub checks 👼
Hopefully this'll also fix #14 |
35276fe
to
fce15cb
Compare
…ruct unexported data fields
Thanks @svyotov. After a long wait, I've decided to go ahead. |
This fixes #103 also fixes #90 however I am not sure it is enough as I see other issues with map.