From a4910b4135c81fcf89cec05cee2ebc03cb2cb835 Mon Sep 17 00:00:00 2001 From: Matthias Diester Date: Mon, 20 May 2019 16:31:07 +0200 Subject: [PATCH] Fix staticcheck/gocyclo findings Introduce PathElement functions to determine their respective function, for example whether they point to a map, or a simple list. Use new helper functions in grab function to make the code easier to read. Introduce grabByPath function to be used if a Path object is already available. Fix error message that start with an upper-case character. Use typed objects in switch .(type) constructs to avoid casting in cases. --- pkg/v1/ytbx/common.go | 4 +-- pkg/v1/ytbx/convert.go | 36 +++++++++----------- pkg/v1/ytbx/getting.go | 14 +++++--- pkg/v1/ytbx/input.go | 6 ++-- pkg/v1/ytbx/list_functions.go | 4 +-- pkg/v1/ytbx/path.go | 25 +++++++++++--- pkg/v1/ytbx/path_test.go | 62 +++++++++++++++++++++------------- pkg/v1/ytbx/ytbx_suite_test.go | 10 +++--- 8 files changed, 95 insertions(+), 66 deletions(-) diff --git a/pkg/v1/ytbx/common.go b/pkg/v1/ytbx/common.go index b509696..e07d478 100644 --- a/pkg/v1/ytbx/common.go +++ b/pkg/v1/ytbx/common.go @@ -36,12 +36,12 @@ const ( // GetType returns the type of the input value with a YAML specific view func GetType(value interface{}) string { - switch value.(type) { + switch tobj := value.(type) { case yaml.MapSlice: return typeMap case []interface{}: - if IsComplexSlice(value.([]interface{})) { + if IsComplexSlice(tobj) { return typeComplexList } diff --git a/pkg/v1/ytbx/convert.go b/pkg/v1/ytbx/convert.go index 2b3d772..77432c5 100644 --- a/pkg/v1/ytbx/convert.go +++ b/pkg/v1/ytbx/convert.go @@ -39,32 +39,29 @@ import ( // `github.com/virtuald/go-ordered-json` and will translate this structure into // the compatible YAML structure. func mapSlicify(obj interface{}) interface{} { - switch obj.(type) { + switch tobj := obj.(type) { case ordered.OrderedObject: - orderedObj := obj.(ordered.OrderedObject) - result := make(yaml.MapSlice, 0, len(orderedObj)) - for _, member := range orderedObj { + result := make(yaml.MapSlice, 0, len(tobj)) + for _, member := range tobj { result = append(result, yaml.MapItem{Key: member.Key, Value: mapSlicify(member.Value)}) } return result case map[string]interface{}: - return mapToYamlSlice(obj.(map[string]interface{})) + return mapToYamlSlice(tobj) case []interface{}: - list := obj.([]interface{}) - result := make([]interface{}, len(list)) - for idx, entry := range list { + result := make([]interface{}, len(tobj)) + for idx, entry := range tobj { result[idx] = mapSlicify(entry) } return result case []map[string]interface{}: - list := obj.([]map[string]interface{}) - result := make([]yaml.MapSlice, len(list)) - for idx, entry := range list { + result := make([]yaml.MapSlice, len(tobj)) + for idx, entry := range tobj { result[idx] = mapToYamlSlice(entry) } @@ -92,21 +89,20 @@ func mapToYamlSlice(input map[string]interface{}) yaml.MapSlice { } func castAsComplexList(obj interface{}) ([]yaml.MapSlice, bool) { - switch obj.(type) { + switch tobj := obj.(type) { case []yaml.MapSlice: - return obj.([]yaml.MapSlice), true + return tobj, true case []interface{}: - list := obj.([]interface{}) - if IsComplexSlice(list) { - result := make([]yaml.MapSlice, len(list)) - for idx, entry := range list { - switch entry.(type) { + if IsComplexSlice(tobj) { + result := make([]yaml.MapSlice, len(tobj)) + for idx, entry := range tobj { + switch x := entry.(type) { case yaml.MapSlice: - result[idx] = entry.(yaml.MapSlice) + result[idx] = x case map[string]interface{}: - result[idx] = mapToYamlSlice(entry.(map[string]interface{})) + result[idx] = mapToYamlSlice(x) } } diff --git a/pkg/v1/ytbx/getting.go b/pkg/v1/ytbx/getting.go index 9106ec4..1d98631 100644 --- a/pkg/v1/ytbx/getting.go +++ b/pkg/v1/ytbx/getting.go @@ -33,12 +33,16 @@ func Grab(obj interface{}, pathString string) (interface{}, error) { return nil, err } - pointer := obj - pointerPath := Path{DocumentIdx: path.DocumentIdx} + return grabByPath(obj, path) +} + +func grabByPath(obj interface{}, path Path) (interface{}, error) { + pointer, pointerPath := obj, Path{DocumentIdx: path.DocumentIdx} + for _, element := range path.PathElements { switch { // Key/Value Map, where the element name is the key for the map - case len(element.Key) == 0 && len(element.Name) > 0: + case element.isMapElement(): if !isMapSlice(pointer) { return nil, fmt.Errorf("failed to traverse tree, expected a %s but found type %s at %s", typeMap, GetType(pointer), pointerPath.ToGoPatchStyle()) } @@ -51,7 +55,7 @@ func Grab(obj interface{}, pathString string) (interface{}, error) { pointer = entry // Complex List, where each list entry is a Key/Value map and the entry is identified by name using an indentifier (e.g. name, key, or id) - case len(element.Key) > 0 && len(element.Name) > 0: + case element.isComplexListElement(): complexList, ok := castAsComplexList(pointer) if !ok { return nil, fmt.Errorf("failed to traverse tree, expected a %s but found type %s at %s", typeComplexList, GetType(pointer), pointerPath.ToGoPatchStyle()) @@ -65,7 +69,7 @@ func Grab(obj interface{}, pathString string) (interface{}, error) { pointer = entry // Simple List (identified by index) - case len(element.Key) == 0 && len(element.Name) == 0: + case element.isSimpleListElement(): if !isList(pointer) { return nil, fmt.Errorf("failed to traverse tree, expected a %s but found type %s at %s", typeSimpleList, GetType(pointer), pointerPath.ToGoPatchStyle()) } diff --git a/pkg/v1/ytbx/input.go b/pkg/v1/ytbx/input.go index 70681f0..480f62a 100644 --- a/pkg/v1/ytbx/input.go +++ b/pkg/v1/ytbx/input.go @@ -256,7 +256,7 @@ func LoadJSONDocuments(input []byte) ([]interface{}, error) { values[i] = value default: - return nil, fmt.Errorf("Unsupported type %s in load document function", types[i]) + return nil, fmt.Errorf("unsupported type %s in load document function", types[i]) } } @@ -318,7 +318,7 @@ func LoadYAMLDocuments(input []byte) ([]interface{}, error) { values[i] = value default: - return nil, fmt.Errorf("Unsupported type %s in load document function", types[i]) + return nil, fmt.Errorf("unsupported type %s in load document function", types[i]) } } @@ -368,7 +368,7 @@ func getBytesFromLocation(location string) ([]byte, error) { } // In any other case, bail out ... - return nil, fmt.Errorf("Unable to get any content using location %s: it is not a file or usable URI", location) + return nil, fmt.Errorf("unable to get any content using location %s: it is not a file or usable URI", location) } // IsStdin checks whether the provided input location refers to the dash diff --git a/pkg/v1/ytbx/list_functions.go b/pkg/v1/ytbx/list_functions.go index 750fb5a..3782197 100644 --- a/pkg/v1/ytbx/list_functions.go +++ b/pkg/v1/ytbx/list_functions.go @@ -31,9 +31,9 @@ func GetIdentifierFromNamedList(list []interface{}) string { counters := map[interface{}]int{} for _, sliceEntry := range list { - switch sliceEntry.(type) { + switch mapslice := sliceEntry.(type) { case yaml.MapSlice: - for _, mapSliceEntry := range sliceEntry.(yaml.MapSlice) { + for _, mapSliceEntry := range mapslice { if _, ok := counters[mapSliceEntry.Key]; !ok { counters[mapSliceEntry.Key] = 0 } diff --git a/pkg/v1/ytbx/path.go b/pkg/v1/ytbx/path.go index a415690..1085ae8 100644 --- a/pkg/v1/ytbx/path.go +++ b/pkg/v1/ytbx/path.go @@ -227,22 +227,22 @@ func ListPaths(location string, style PathStyle) ([]Path, error) { } func traverseTree(path Path, obj interface{}, leafFunc func(path Path, value interface{})) { - switch obj.(type) { + switch tobj := obj.(type) { case []interface{}: - if identifier := GetIdentifierFromNamedList(obj.([]interface{})); identifier != "" { - for _, entry := range obj.([]interface{}) { + if identifier := GetIdentifierFromNamedList(tobj); identifier != "" { + for _, entry := range tobj { name, data := splitEntryIntoNameAndData(entry.(yaml.MapSlice), identifier) traverseTree(NewPathWithNamedListElement(path, identifier, name), data, leafFunc) } } else { - for idx, entry := range obj.([]interface{}) { + for idx, entry := range tobj { traverseTree(NewPathWithIndexedListElement(path, idx), entry, leafFunc) } } case yaml.MapSlice: - for _, mapitem := range obj.(yaml.MapSlice) { + for _, mapitem := range tobj { traverseTree(NewPathWithNamedElement(path, mapitem.Key), mapitem.Value, leafFunc) } @@ -379,3 +379,18 @@ func ParsePathString(pathString string, obj interface{}) (Path, error) { return ParseDotStylePathString(pathString, obj) } + +func (element PathElement) isMapElement() bool { + return len(element.Key) == 0 && + len(element.Name) > 0 +} + +func (element PathElement) isComplexListElement() bool { + return len(element.Key) > 0 && + len(element.Name) > 0 +} + +func (element PathElement) isSimpleListElement() bool { + return len(element.Key) == 0 && + len(element.Name) == 0 +} diff --git a/pkg/v1/ytbx/path_test.go b/pkg/v1/ytbx/path_test.go index 39fdff8..ba37af4 100644 --- a/pkg/v1/ytbx/path_test.go +++ b/pkg/v1/ytbx/path_test.go @@ -152,20 +152,29 @@ var _ = Describe("path tests", func() { list, err := ComparePaths(assetsDirectory+"/testbed/sample_a.yml", assetsDirectory+"/testbed/sample_b.yml", GoPatchStyle, false) Expect(err).ToNot(HaveOccurred()) - listOfPaths := make([]Path, 5) - listOfPaths = []Path{{DocumentIdx: 0, PathElements: []PathElement{ - {Idx: -1, Key: "", Name: "yaml"}, - {Idx: -1, Key: "", Name: "structure"}, - {Idx: -1, Key: "", Name: "somekey"}, - }}, {DocumentIdx: 0, PathElements: []PathElement{ - {Idx: -1, Key: "", Name: "yaml"}, - {Idx: -1, Key: "", Name: "structure"}, - {Idx: -1, Key: "", Name: "dot"}, - }}, {DocumentIdx: 0, PathElements: []PathElement{ - {Idx: -1, Key: "", Name: "list"}, - {Idx: -1, Key: "name", Name: "sametwo"}, - {Idx: -1, Key: "", Name: "somekey"}, - }}} + listOfPaths := []Path{ + { + DocumentIdx: 0, PathElements: []PathElement{ + {Idx: -1, Key: "", Name: "yaml"}, + {Idx: -1, Key: "", Name: "structure"}, + {Idx: -1, Key: "", Name: "somekey"}, + }, + }, + { + DocumentIdx: 0, PathElements: []PathElement{ + {Idx: -1, Key: "", Name: "yaml"}, + {Idx: -1, Key: "", Name: "structure"}, + {Idx: -1, Key: "", Name: "dot"}, + }, + }, + { + DocumentIdx: 0, PathElements: []PathElement{ + {Idx: -1, Key: "", Name: "list"}, + {Idx: -1, Key: "name", Name: "sametwo"}, + {Idx: -1, Key: "", Name: "somekey"}, + }, + }, + } Expect(list).To(BeEquivalentTo(listOfPaths)) }) @@ -174,16 +183,21 @@ var _ = Describe("path tests", func() { list, err := ComparePaths(assetsDirectory+"/testbed/sample_a.yml", assetsDirectory+"/testbed/sample_b.yml", GoPatchStyle, true) Expect(err).ToNot(HaveOccurred()) - listOfPathsWithSameValue := make([]Path, 5) - listOfPathsWithSameValue = []Path{{DocumentIdx: 0, PathElements: []PathElement{ - {Idx: -1, Key: "", Name: "yaml"}, - {Idx: -1, Key: "", Name: "structure"}, - {Idx: -1, Key: "", Name: "dot"}, - }}, {DocumentIdx: 0, PathElements: []PathElement{ - {Idx: -1, Key: "", Name: "list"}, - {Idx: -1, Key: "name", Name: "sametwo"}, - {Idx: -1, Key: "", Name: "somekey"}, - }}} + listOfPathsWithSameValue := []Path{ + { + DocumentIdx: 0, PathElements: []PathElement{ + {Idx: -1, Key: "", Name: "yaml"}, + {Idx: -1, Key: "", Name: "structure"}, + {Idx: -1, Key: "", Name: "dot"}, + }}, + { + DocumentIdx: 0, PathElements: []PathElement{ + {Idx: -1, Key: "", Name: "list"}, + {Idx: -1, Key: "name", Name: "sametwo"}, + {Idx: -1, Key: "", Name: "somekey"}, + }, + }, + } Expect(list).To(BeEquivalentTo(listOfPathsWithSameValue)) }) diff --git a/pkg/v1/ytbx/ytbx_suite_test.go b/pkg/v1/ytbx/ytbx_suite_test.go index 8aeb485..72816f1 100644 --- a/pkg/v1/ytbx/ytbx_suite_test.go +++ b/pkg/v1/ytbx/ytbx_suite_test.go @@ -114,9 +114,9 @@ func yml(input string) yaml.MapSlice { // Load YAML by parsing the actual string as YAML if it was not a file location doc := singleDoc(input) - switch doc.(type) { + switch mapslice := doc.(type) { case yaml.MapSlice: - return doc.(yaml.MapSlice) + return mapslice } Fail(fmt.Sprintf("Failed to use YAML, parsed data is not a YAML MapSlice:\n%s\n", input)) @@ -126,12 +126,12 @@ func yml(input string) yaml.MapSlice { func list(input string) []interface{} { doc := singleDoc(input) - switch doc.(type) { + switch tobj := doc.(type) { case []interface{}: - return doc.([]interface{}) + return tobj case []yaml.MapSlice: - return ytbx.SimplifyList(doc.([]yaml.MapSlice)) + return ytbx.SimplifyList(tobj) } Fail(fmt.Sprintf("Failed to use YAML, parsed data is not a slice of any kind:\n%s\nIt was parsed as: %#v", input, doc))