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

fixes issue #187 #253

Merged
merged 4 commits into from
Aug 17, 2024
Merged

fixes issue #187 #253

merged 4 commits into from
Aug 17, 2024

Conversation

vsemichev
Copy link
Contributor

Hi, this PR will hopefully fix the issue #187 with map destinations for Map method. It's one simple change.

@mallardduck
Copy link

Based on #246 - I'm not sure this will ever be accepted.

@darccio
Copy link
Owner

darccio commented Aug 1, 2024

@mallardduck This PR is missing tests. I asked the original contributor to add them but I got no answer.

@vsemichev
Copy link
Contributor Author

Hi @darccio, sorry for the delay. I've added a couple of test to verify the fix. Also, a slight statement change to take into account the case where you want to override map values even when the source structure fields are empty.

@darccio
Copy link
Owner

darccio commented Aug 17, 2024

@vsemichev Looks it works and all tests are green. Let's release another v1 version.

@darccio darccio merged commit 96f24af into darccio:master Aug 17, 2024
3 checks passed
@@ -58,7 +58,7 @@ func deepMap(dst, src reflect.Value, visited map[uintptr]*visit, depth int, conf
}
fieldName := field.Name
fieldName = changeInitialCase(fieldName, unicode.ToLower)
if v, ok := dstMap[fieldName]; !ok || (isEmptyValue(reflect.ValueOf(v), !config.ShouldNotDereference) || overwrite) {
if _, ok := dstMap[fieldName]; !ok || (!isEmptyValue(reflect.ValueOf(src.Field(i).Interface()), !config.ShouldNotDereference) && overwrite) || config.overwriteWithEmptyValue {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this change would break our use case, consider the following scenario

	dst := map[string]interface{}{
		"foo":   nil,
		"bar":   1,
	}
	src := struct {
		Foo   *string
		Bar   int
	}{
		Foo: &"hello",
		Bar: 42,
	}
	if err := mergo.Map(&dst, src); err != nil {
		t.Error(err)
	}

we expect *dst["foo"] == "hello" and dst["bar"] == 1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankdvd Your use case seems to be an unintended consequence. Mergo shouldn't flatten structures while mapping.

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

Successfully merging this pull request may close these issues.

4 participants