From fce15cb6fcf217b17e5caa7b0b7932062f4637b7 Mon Sep 17 00:00:00 2001 From: Stanislav Yotov <29090864+svyotov@users.noreply.github.com> Date: Thu, 14 Mar 2019 02:38:29 +0100 Subject: [PATCH] Reduce code complexity --- issue66_test.go | 2 +- merge.go | 107 ++++++++++++++++-------------------------------- mergo_test.go | 23 ++++++++--- 3 files changed, 54 insertions(+), 78 deletions(-) diff --git a/issue66_test.go b/issue66_test.go index 9e4bcce..ebc4276 100644 --- a/issue66_test.go +++ b/issue66_test.go @@ -21,7 +21,7 @@ func TestPrivateSlice(t *testing.T) { t.Fatalf("Error during the merge: %v", err) } if len(p1.PublicStrings) != 3 { - t.Error("5 elements should be in 'PublicStrings' field") + t.Error("3 elements should be in 'PublicStrings' field, when no append") } if len(p1.privateStrings) != 2 { t.Error("2 elements should be in 'privateStrings' field") diff --git a/merge.go b/merge.go index 194f9b0..b8d7299 100644 --- a/merge.go +++ b/merge.go @@ -17,10 +17,8 @@ import ( func hasExportedField(dst reflect.Value) (exported bool) { for i, n := 0, dst.NumField(); i < n; i++ { field := dst.Type().Field(i) - if field.Anonymous && dst.Field(i).Kind() == reflect.Struct { - exported = exported || hasExportedField(dst.Field(i)) - } else { - exported = exported || isFieldExported(field) + if isFieldExported(field) { + return true } } return @@ -68,6 +66,7 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, if !src.IsValid() { return } + if dst.CanAddr() { addr := dst.UnsafeAddr() h := 17 * addr @@ -89,6 +88,11 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, } } + if dst.IsValid() && src.IsValid() && src.Type() != dst.Type() { + err = fmt.Errorf("cannot append two different types (%s, %s)", src.Kind(), dst.Kind()) + return + } + switch dst.Kind() { case reflect.Struct: if hasExportedField(dst) { @@ -121,11 +125,13 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, } else { dst = dstCp } + return } else { if (isReflectNil(dst) || overwrite) && (!isEmptyValue(src) || overwriteWithEmptySrc) { dst = src } } + case reflect.Map: if dst.IsNil() && !src.IsNil() { if dst.CanSet() { @@ -137,91 +143,52 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, } for _, key := range src.MapKeys() { srcElement := src.MapIndex(key) + dstElement := dst.MapIndex(key) if !srcElement.IsValid() { continue } - dstElement := dst.MapIndex(key) if dst.MapIndex(key).IsValid() { k := dstElement.Interface() dstElement = reflect.ValueOf(k) } - - switch srcElement.Kind() { - case reflect.Chan, reflect.Func, reflect.Map, reflect.Interface, reflect.Slice: - if srcElement.IsNil() { - continue - } - fallthrough - default: - if !srcElement.CanInterface() { - continue - } - switch reflect.TypeOf(srcElement.Interface()).Kind() { - case reflect.Struct: - fallthrough - case reflect.Ptr: - fallthrough - case reflect.Map: - srcMapElm := srcElement - dstMapElm := dstElement - if srcMapElm.CanInterface() { - srcMapElm = reflect.ValueOf(srcMapElm.Interface()) - if dstMapElm.IsValid() { - dstMapElm = reflect.ValueOf(dstMapElm.Interface()) - } - } - dstMapElm, err = deepMerge(dstMapElm, srcMapElm, visited, depth+1, config) - if err != nil { - return - } - dst.SetMapIndex(key, dstMapElm) - case reflect.Slice: - srcSlice := reflect.ValueOf(srcElement.Interface()) - - var dstSlice reflect.Value - if !dstElement.IsValid() || isReflectNil(dstElement) { - dstSlice = reflect.MakeSlice(srcSlice.Type(), 0, srcSlice.Len()) - } else { - dstSlice = reflect.ValueOf(dstElement.Interface()) - } - - if (!isEmptyValue(src) || overwriteWithEmptySrc || overwriteSliceWithEmptySrc) && (overwrite || isEmptyValue(dst)) && !config.AppendSlice { - if typeCheck && srcSlice.Type() != dstSlice.Type() { - return fmt.Errorf("cannot override two slices with different type (%s, %s)", srcSlice.Type(), dstSlice.Type()) - } - dstSlice = srcSlice - } else if config.AppendSlice { - if srcSlice.Type() != dstSlice.Type() { - return fmt.Errorf("cannot append two slices with different type (%s, %s)", srcSlice.Type(), dstSlice.Type()) - } - dstSlice = reflect.AppendSlice(dstSlice, srcSlice) - } - dst.SetMapIndex(key, dstSlice) + if isReflectNil(srcElement) { + if overwrite || isReflectNil(dstElement) { + dst.SetMapIndex(key, srcElement) } + continue } - if dstElement.IsValid() && !isEmptyValue(dstElement) && (reflect.TypeOf(srcElement.Interface()).Kind() == reflect.Map || reflect.TypeOf(srcElement.Interface()).Kind() == reflect.Slice) || (reflect.TypeOf(srcElement.Interface()).Kind() == reflect.Struct) { + if !srcElement.CanInterface() { continue } - if srcElement.IsValid() && ((srcElement.Kind() != reflect.Ptr && overwrite) || !dstElement.IsValid() || isEmptyValue(dstElement)) { - if dst.IsNil() { - dst.Set(reflect.MakeMap(dst.Type())) + if srcElement.CanInterface() { + srcElement = reflect.ValueOf(srcElement.Interface()) + if dstElement.IsValid() { + dstElement = reflect.ValueOf(dstElement.Interface()) } - dst.SetMapIndex(key, srcElement) } + dstElement, err = deepMerge(dstElement, srcElement, visited, depth+1, config) + if err != nil { + return + } + dst.SetMapIndex(key, dstElement) + } case reflect.Slice: - if !dst.CanSet() { - break - } - if (!isEmptyValue(src) || overwriteWithEmptySrc || overwriteSliceWithEmptySrc) && (overwrite || isEmptyValue(dst)) && !config.AppendSlice { - dst.Set(src) + newSlice := dst + if (!isEmptyValue(src) || overwriteWithEmptySrc) && (overwrite || isEmptyValue(dst)) && !config.AppendSlice { + newSlice = src } else if config.AppendSlice { if src.Type() != dst.Type() { err = fmt.Errorf("cannot append two slice with different type (%s, %s)", src.Type(), dst.Type()) return } - dst.Set(reflect.AppendSlice(dst, src)) + newSlice = reflect.AppendSlice(dst, src) + } + if dst.CanSet() { + dst.Set(newSlice) + } else { + dst = newSlice } case reflect.Ptr, reflect.Interface: if isReflectNil(src) { @@ -237,7 +204,6 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, dst = src } } -<<<<<<< HEAD } break } @@ -247,9 +213,6 @@ func deepMerge(dstIn, src reflect.Value, visited map[uintptr]*visit, depth int, if dst.CanSet() && (overwrite || isEmptyValue(dst)) { dst.Set(src) } -======= - ->>>>>>> semi working } else if src.Kind() == reflect.Ptr { if dst, err = deepMerge(dst.Elem(), src.Elem(), visited, depth+1, config); err != nil { return diff --git a/mergo_test.go b/mergo_test.go index 02e9705..23eeb31 100644 --- a/mergo_test.go +++ b/mergo_test.go @@ -345,13 +345,13 @@ func TestEmptyToNotEmptyMaps(t *testing.T) { } func TestMapsWithOverwrite(t *testing.T) { - m := map[string]simpleTest{ + dst := map[string]simpleTest{ "a": {}, // overwritten by 16 "b": {42}, // overwritten by 0, as map Value is not addressable and it doesn't check for b is set or not set in `n` "c": {13}, // overwritten by 12 "d": {61}, } - n := map[string]simpleTest{ + src := map[string]simpleTest{ "a": {16}, "b": {}, "c": {12}, @@ -365,12 +365,12 @@ func TestMapsWithOverwrite(t *testing.T) { "e": {14}, } - if err := MergeWithOverwrite(&m, n); err != nil { + if err := MergeWithOverwrite(&dst, src); err != nil { t.Fatalf(err.Error()) } - if !reflect.DeepEqual(m, expect) { - t.Fatalf("Test failed:\ngot :\n%#v\n\nwant :\n%#v\n\n", m, expect) + if !reflect.DeepEqual(dst, expect) { + t.Fatalf("Test failed:\ngot :\n%#v\n\nwant :\n%#v\n\n", dst, expect) } } @@ -885,6 +885,7 @@ func TestBooleanPointer(t *testing.T) { } func TestMergeMapWithInnerSliceOfDifferentType(t *testing.T) { +<<<<<<< HEAD testCases := []struct { name string options []func(*Config) @@ -900,6 +901,13 @@ func TestMergeMapWithInnerSliceOfDifferentType(t *testing.T) { []func(*Config){WithOverride, WithTypeCheck}, "cannot override two slices with different type", }, +======= + dst := map[string]interface{}{ + "foo": []string{"a", "b"}, + } + src := map[string]interface{}{ + "foo": []int{1, 2}, +>>>>>>> Reduce code complexity } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -910,10 +918,15 @@ func TestMergeMapWithInnerSliceOfDifferentType(t *testing.T) { "foo": []int{1, 2}, } +<<<<<<< HEAD if err := Merge(&src, &dst, tc.options...); err == nil || !strings.Contains(err.Error(), tc.err) { t.Fatalf("expected %q, got %q", tc.err, err) } }) +======= + if err := Merge(&dst, src, WithOverride, WithAppendSlice); err == nil { + t.Fatal("expected an error, got nothing") +>>>>>>> Reduce code complexity } }