Merge branch 'fix-bad-walkerror'

* fix-bad-walkerror:
  Add an additional test around Map and nil nodes
  Only use Map() node result if no error is returned
  Add test for #12 - nil WalkError.Node panics

Author: nilium

walk.go

@@ -42,6 +42,12 @@ type WalkExiter interface {
 //
 // Walk will return a *WalkError if any error occurs during a walk. The WalkError will contain both
 // the parent and child node that the error occurred for.
+//
+// If the walker is a WalkMapper, any error in attempting to map a node will return a WalkError with
+// the original node, not any resulting node. If the mapping is to a nil node without error, the
+// node is deleted from the parent.
+//
+// Nil child nodes are skipped.
 func Walk(parent ParentNode, walker Walker) (err error) {
 	return walkInContext(parent, parent, walker)
 }
@@ -53,24 +59,30 @@ func walkInContext(context, parent ParentNode, walker Walker) (err error) {
 
 	for i := 0; i < len(children); i++ {
 		child := children[i]
+		if child == nil {
+			// Skip over nil nodes because they're mostly harmless -- you just can't act
+			// on them at all.
+			continue
+		}
 
 		// Remap the child node if the walker implemented WalkMapper
 		if mapper != nil {
-			child, err = mapper.Map(child)
+			var newChild Node
+			newChild, err = mapper.Map(child)
 			if err != nil {
 				return walkErr(parent, context, child, err)
 			}
 
-			// If the new child is nil, remove it from the slice of children
-			if child == nil {
+			// If the new child is nil, remove the original child from the slice of children
+			if newChild == nil {
 				copy(children[i:], children[i+1:])
 				children[len(children)-1] = nil
 				children = children[:len(children)-1]
 				// Now that the next child is in this child's place, walk i back one cell
 				i--
 				continue
 			}
-			children[i] = child
+			children[i] = newChild
 		}
 
 		switch child := child.(type) {

walk_test.go

@@ -1,11 +1,23 @@
 package codf
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
 )
 
+type mapFunc func(Node) (Node, error)
+
+var _ WalkMapper = mapFunc(nil)
+
+func (f mapFunc) Map(n Node) (Node, error) {
+	return f(n)
+}
+
+func (f mapFunc) Statement(*Statement) error            { return nil }
+func (f mapFunc) EnterSection(*Section) (Walker, error) { return f, nil }
+
 type docWalker struct {
 	name       string
 	statements map[string]struct{}
@@ -313,3 +325,36 @@ func TestWalkMapper(t *testing.T) {
 		t.Errorf("statements = %q; want %q", walker.statements, wantStatements)
 	}
 }
+
+func TestMapDelete(t *testing.T) {
+	const DocSource = `stmt 1; section arg {}`
+
+	defer setlogf(t)()
+
+	doc := mustParseNamed(t, "root.conf", DocSource)
+	err := Walk(doc, mapFunc(func(Node) (Node, error) { return nil, nil }))
+	if err != nil {
+		t.Fatalf("err = %v; want nil", err)
+	}
+
+	if got := doc.Children; len(got) != 0 {
+		t.Fatalf("children = %#v; want an empty slice", got)
+	}
+}
+
+func TestMapError(t *testing.T) {
+	const wantErrMessage = `[1:1:0] root.conf: stmt in main: expected error`
+	const DocSource = `stmt 1; section arg {}`
+
+	defer setlogf(t)()
+
+	doc := mustParseNamed(t, "root.conf", DocSource)
+	doc.Children = append([]Node{nil}, doc.Children...)
+
+	wantErr := errors.New("expected error")
+	err := Walk(doc, mapFunc(func(Node) (Node, error) { return nil, wantErr }))
+
+	if got := err.Error(); got != wantErrMessage {
+		t.Fatalf("err = %q; want %q", got, wantErrMessage)
+	}
+}