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 Struct in Map #105

Merged
merged 21 commits into from Mar 24, 2020
Merged

Conversation

svyotov
Copy link
Contributor

@svyotov svyotov commented Mar 4, 2019

This fixes #103 also fixes #90 however I am not sure it is enough as I see other issues with map.

Copy link
Collaborator

@vdemeester vdemeester left a 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}}

issue90_test.go Outdated Show resolved Hide resolved
issue90_test.go Outdated Show resolved Hide resolved
@svyotov
Copy link
Contributor Author

svyotov commented Mar 4, 2019

@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.

merge.go Outdated Show resolved Hide resolved
merge.go Show resolved Hide resolved
@svyotov
Copy link
Contributor Author

svyotov commented Mar 5, 2019

@vdemeester go test ./... is passing for me, however I have just started understanding reflect in Golang so I appreciate there might be a much better way of doing this. I changed some of the unit tests as isNil returns true for int when equal to 0 and some of the changes should be done.
If you wish to not use this PR and close it that is fine by me, though IMO at the very least issue90_test.go should be reused as the tests there is failing with the code currently on master.

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.

mergo_test.go Show resolved Hide resolved
mergo_test.go Show resolved Hide resolved
@svyotov
Copy link
Contributor Author

svyotov commented Mar 11, 2019

Any thoughts on this PR @vdemeester, @imdario?

@darccio
Copy link
Owner

darccio commented Mar 11, 2019

I'll check it this evening.

mergo_test.go Show resolved Hide resolved
Copy link
Collaborator

@vdemeester vdemeester left a 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 👼

issue90_test.go Outdated Show resolved Hide resolved
issue90_test.go Outdated Show resolved Hide resolved
merge.go Show resolved Hide resolved
merge.go Outdated Show resolved Hide resolved
@boosh
Copy link

boosh commented Mar 31, 2019

Hopefully this'll also fix #14

@darccio
Copy link
Owner

darccio commented Mar 24, 2020

Thanks @svyotov. After a long wait, I've decided to go ahead.

@darccio darccio merged commit 66f88b4 into darccio:master Mar 24, 2020
@svyotov svyotov deleted the fix-panic-on-merge branch April 29, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants