Repository: openconfig/goyang
Pull request #175 (2021-02-18 22:30:14)

Polish code in entry.go
- Add a few tests.
- Remove redunant code (`entryCache[n] = e`).
- Remove unreachable code (`if e.Kind == LeafEntry`)
- Improve naming and complexity a little.
- Improve comments

coveralls: 
[![Coverage Status](https://coveralls.io/builds/37254363/badge)](https://coveralls.io/builds/37254363)

Coverage increased (+0.3%) to 81.415% when pulling **b6ac2385ea1f7bb7b103a8b28187970f4dd935e2 on polish-entry** into **1ad08a59f3677cc920ae59d3460e6f36cb9ccd1a on add-module-tests**.

wenovus: @robshakir for simplicity I put all the PRs in a chain, so can't merge until the previous ones have been approved. Starts at #160.

Patch:
diff --git a/pkg/yang/entry.go b/pkg/yang/entry.go
index faa8d9d..f178d3f 100644
--- a/pkg/yang/entry.go
+++ b/pkg/yang/entry.go
@@ -71,9 +71,13 @@ type deviationPresence struct {
 	hasMaxElements bool
 }
 
-// An Entry represents a single node (directory or leaf) created from the
-// AST.  Directory entries have a non-nil Dir entry.  Leaf nodes have a nil
-// Dir entry.  If Errors is not nil then the only other valid field is Node.
+// Entry represents a single schema tree node, which can be a directory
+// (containing a subtree) or a leaf node (which contains YANG types that have
+// no children, e.g., leaf, leaf-list). They can be distinguished by whether
+// their "Dir" field is nil. This object is created from a corresponding AST
+// node after applying modifications (i.e. uses, augments, deviations). If
+// Errors is not nil then it means semantic errors existed while converting the
+// AST, in which case the only other valid field other than Errors is Node.
 type Entry struct {
 	Parent      *Entry    `json:"-"`
 	Node        Node      `json:"-"` // the base node this Entry was derived from.
@@ -92,8 +96,10 @@ type Entry struct {
 	Key string            `json:",omitempty"` // Optional key name for lists (i.e., maps)
 
 	// Fields associated with leaf nodes
-	Type *YangType    `json:",omitempty"`
-	Exts []*Statement `json:",omitempty"` // extensions found
+	Type *YangType `json:",omitempty"`
+
+	// Extensions found
+	Exts []*Statement `json:",omitempty"`
 
 	// Fields associated with list nodes (both lists and leaf-lists)
 	ListAttr *ListAttr `json:",omitempty"`
@@ -301,7 +307,7 @@ func newLeaf(n Node) *Entry {
 	}
 }
 
-// newError returns an error node using format and v to create the error
+// newError returns an error Entry using format and v to create the error
 // contained in the node.  The location of the error is prepended.
 func newError(n Node, format string, v ...interface{}) *Entry {
 	e := &Entry{Node: n}
@@ -309,7 +315,7 @@ func newError(n Node, format string, v ...interface{}) *Entry {
 	return e
 }
 
-// errorf appends the entry constructed from string and v to the list of errors
+// errorf appends the error constructed from string and v to the list of errors
 // on e.
 func (e *Entry) errorf(format string, v ...interface{}) {
 	e.Errors = append(e.Errors, fmt.Errorf(format, v...))
@@ -529,7 +535,7 @@ func semCheckMinElements(v *Value) (uint64, error) {
 // if there were any errors.
 func ToEntry(n Node) (e *Entry) {
 	if n == nil {
-		err := errors.New("ToEntry called with nil")
+		err := errors.New("ToEntry called on nil AST node")
 		return &Entry{
 			Node:   &ErrorNode{Error: err},
 			Errors: []error{err},
@@ -583,7 +589,6 @@ func ToEntry(n Node) (e *Entry) {
 			e.Default = s.Default.Name
 		}
 		e.Type = s.Type.YangType
-		entryCache[n] = e
 		e.Config, err = tristateValue(s.Config)
 		e.addError(err)
 		e.Prefix = getRootPrefix(e)
@@ -610,7 +615,7 @@ func ToEntry(n Node) (e *Entry) {
 			When:        s.When,
 		}
 
-		e := ToEntry(leaf)
+		e = ToEntry(leaf)
 		e.ListAttr = NewDefaultListAttr()
 		e.ListAttr.OrderedBy = s.OrderedBy
 		var err error
@@ -631,7 +636,7 @@ func ToEntry(n Node) (e *Entry) {
 		// We need to return a duplicate so we resolve properly
 		// when the group is used in multiple locations and the
 		// grouping has a leafref that references outside the group.
-		e := ToEntry(g).dup()
+		e = ToEntry(g).dup()
 		addExtraKeywordsToLeafEntry(n, e)
 		return e
 	}
@@ -674,8 +679,7 @@ func ToEntry(n Node) (e *Entry) {
 		e.Kind = DeviateEntry
 	}
 
-	// Use Elem to get the Value of structure that n is pointing to, not
-	// the Value of the pointer.
+	// Use Elem to get the Value of structure that n is pointing to.
 	v := reflect.ValueOf(n).Elem()
 	t := v.Type()
 	found := false
@@ -739,8 +743,8 @@ func ToEntry(n Node) (e *Entry) {
 				e.importErrors(ToEntry(a))
 			}
 		case "import":
-			// Apparently import only makes types and such
-			// available.  There is nothing else for us to do.
+			// Import only makes types and such available.
+			// There is nothing else for us to do.
 		case "include":
 			for _, a := range fv.Interface().([]*Include) {
 				// Handle circular dependencies between submodules. This can occur in
@@ -873,11 +877,6 @@ func ToEntry(n Node) (e *Entry) {
 		// Keywords that do not need to be handled as an Entry as they are added
 		// to other dictionaries.
 		case "default":
-			if e.Kind == LeafEntry {
-				// default is handled separately for a leaf, but in a deviate statement
-				// we must deal with it here.
-				continue
-			}
 			d, ok := fv.Interface().(*Value)
 			if !ok {
 				e.addError(fmt.Errorf("%s: unexpected default type in %s:%s", Source(n), n.Kind(), n.NName()))
@@ -1050,15 +1049,15 @@ func (e *Entry) Augment(addErrors bool) (processed, skipped int) {
 	// Augments can depend upon augments.  We need to figure out how to
 	// order the augments (or just keep trying until we can make no further
 	// progress)
-	var sa []*Entry
+	var unapplied []*Entry
 	for _, a := range e.Augments {
-		ae := a.Find(a.Name)
-		if ae == nil {
+		target := a.Find(a.Name)
+		if target == nil {
 			if addErrors {
 				e.errorf("%s: augment %s not found", Source(a.Node), a.Name)
 			}
 			skipped++
-			sa = append(sa, a)
+			unapplied = append(unapplied, a)
 			continue
 		}
 		// Augments do not have a prefix we merge in, just a node.
@@ -1066,10 +1065,10 @@ func (e *Entry) Augment(addErrors bool) (processed, skipped int) {
 		// augment since the nodes have this namespace even though they
 		// are merged into another entry.
 		processed++
-		ae.merge(nil, a.Namespace(), a)
-		ae.Augmented = append(ae.Augmented, a.shallowDup())
+		target.merge(nil, a.Namespace(), a)
+		target.Augmented = append(target.Augmented, a.shallowDup())
 	}
-	e.Augments = sa
+	e.Augments = unapplied
 	return processed, skipped
 }
 
@@ -1177,10 +1176,10 @@ func (e *Entry) ApplyDeviate() []error {
 	}
 
 	return errs
-
 }
 
-// FixChoice inserts missing Case entries in a choice
+// FixChoice inserts missing Case entries for non-case entries within a choice
+// entry.
 func (e *Entry) FixChoice() {
 	if e.Kind == ChoiceEntry && len(e.Errors) == 0 {
 		for k, ce := range e.Dir {
@@ -1354,7 +1353,7 @@ func (e *Entry) Namespace() *Value {
 	return new(Value)
 }
 
-// InstantiatingModule returns the YANG module which instanitated the Entry
+// InstantiatingModule returns the YANG module which instantiated the Entry
 // within the schema tree - using the same rules described in the documentation
 // of the Namespace function. The namespace is resolved in the module name. This
 // approach to namespacing is used when serialising YANG-modelled data to JSON as
@@ -1365,11 +1364,11 @@ func (e *Entry) InstantiatingModule() (string, error) {
 		return "", fmt.Errorf("entry %s had nil namespace", e.Name)
 	}
 
-	ns, err := e.Modules().FindModuleByNamespace(n.Name)
+	module, err := e.Modules().FindModuleByNamespace(n.Name)
 	if err != nil {
 		return "", fmt.Errorf("could not find module %q when retrieving namespace for %s", n.Name, e.Name)
 	}
-	return ns.Name, nil
+	return module.Name, nil
 }
 
 // shallowDup makes a shallow duplicate of e (only direct children are
@@ -1489,25 +1488,18 @@ func (s sortedErrors) Less(i, j int) bool {
 		return false
 	}
 
-	// compare compares field x to see which is less.
-	// numbers are compared as numbers.
-	// -1 means less than, 1 means greater or *equal* to and 0 means equal,
-	// thus far, but still undecided and need to see the rest of the elements.
-	compare := func(x int) int {
-		switch {
-		case len(fi) == x && len(fj) > x:
-			return -1
-		case len(fj) == x && len(fi) > x:
-			return 1
-		case len(fj) < x && len(fi) < x: // If neither element exist, then they're equal.
-			return 1
-		}
-		return nless(fi[x-1], fj[x-1]) // Both slices have this index: compare them.
-	}
 	// compare remaining indices of the error string slices
 	// in order to create a total ordering.
-	for x := 1; x < errorSplitCount; x++ {
-		switch compare(x) {
+	for i := 1; i < errorSplitCount; i++ {
+		switch {
+		// Handle when an expected index doesn't exist.
+		case len(fj) == i:
+			return false
+		case len(fi) == i:
+			return true
+		}
+
+		switch nless(fi[i], fj[i]) {
 		case -1:
 			return true
 		case 1:
@@ -1547,7 +1539,7 @@ func errorSort(errors []error) []error {
 // DefaultValue returns the schema default value for e, if any. If the leaf
 // has no explicit default, its type default (if any) will be used.
 func (e *Entry) DefaultValue() string {
-	if len(e.Default) > 0 {
+	if e.Default != "" {
 		return e.Default
 	} else if typ := e.Type; typ != nil {
 		if leaf, ok := e.Node.(*Leaf); ok {


Test patch:
diff --git a/pkg/yang/entry_test.go b/pkg/yang/entry_test.go
index 1d23be5..d8a88bf 100644
--- a/pkg/yang/entry_test.go
+++ b/pkg/yang/entry_test.go
@@ -44,7 +44,7 @@ func TestNilEntry(t *testing.T) {
 		fallthrough
 	case 1:
 		got := errs[0].Error()
-		want := "ToEntry called with nil"
+		want := "ToEntry called on nil AST node"
 		if got != want {
 			t.Fatalf("got error %q, want %q", got, want)
 		}
@@ -1094,6 +1094,20 @@ module defaults {
       type string;
     }
     uses common;
+
+    choice choice-default {
+      case alpha {
+        leaf alpha {
+          type string;
+        }
+      }
+      case zeta {
+        leaf zeta {
+          type string;
+        }
+      }
+      default zeta;
+    }
   }
 
 }
@@ -1136,6 +1150,10 @@ module defaults {
 			path: []string{"defaults", "mandatory-default"},
 			want: "",
 		},
+		{
+			path: []string{"defaults", "choice-default"},
+			want: "zeta",
+		},
 	} {
 		tname := strings.Join(tc.path, "/")
 
@@ -2787,6 +2805,24 @@ func TestDeviation(t *testing.T) {
 				},
 			}},
 		},
+	}, {
+		desc: "error case - deviate type not recognized",
+		inFiles: map[string]string{
+			"deviate": `
+				module deviate {
+					prefix "d";
+					namespace "urn:d";
+
+					leaf a { type string; }
+
+					deviation /a {
+						deviate shrink {
+							max-elements 42;
+						}
+					}
+				}`,
+		},
+		wantProcessErrSubstring: "unknown deviation type",
 	}, {
 		desc: "error case - deviation add max-element to non-list",
 		inFiles: map[string]string{
@@ -3394,6 +3430,28 @@ func TestLeafEntry(t *testing.T) {
 				t.Errorf("got %d, want %d", got, want)
 			}
 		},
+	}, {
+		name: "leaf description",
+		inModules: map[string]string{
+			"test.yang": `
+			module test {
+				prefix "t";
+				namespace "urn:t";
+
+				leaf "mandatory" {
+					type "string" {
+					}
+					description "I am a leaf";
+				}
+			}
+			`,
+		},
+		wantEntryPath: "/test/mandatory",
+		wantEntryCustomTest: func(t *testing.T, e *Entry) {
+			if got, want := e.Description, "I am a leaf"; got != want {
+				t.Errorf("got %q, want %q", got, want)
+			}
+		},
 	}}
 
 	for _, tt := range tests {
@@ -3443,6 +3501,7 @@ func TestLess(t *testing.T) {
 		{"testfile2:1:1", errors.New("test error4")},
 		{"testfile3:1:1:error5", errors.New("test error5")},
 		{"testfile3:1:2:error6", errors.New("test error6")},
+		{"testfile3:1:1:error7", errors.New("test error7")},
 	}
 
 	tests := []struct {
@@ -3515,6 +3574,16 @@ func TestLess(t *testing.T) {
 		i:    5,
 		j:    5,
 		want: false,
+	}, {
+		desc: "compare different strings with four slices",
+		i:    7,
+		j:    5,
+		want: false,
+	}, {
+		desc: "compare different strings with four slices",
+		i:    5,
+		j:    7,
+		want: true,
 	}}
 	var cmpSymbol byte
 	for _, tt := range tests {