From edf6ebbf8ea34e62eccaca3f59f3a67bfb909304 Mon Sep 17 00:00:00 2001 From: Noel Cower Date: Fri, 28 Sep 2018 14:57:54 -0700 Subject: [PATCH 1/3] Add test for #12 - nil WalkError.Node panics This adds a test to confirm that a WalkError returned from Walk(), when produced by a WalkMapper that returns a nil node and error, panics when inspected. This doesn't address WalkErrors panicking on their own since it's necessary for WalkError.Node to be non-nil. --- walk_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/walk_test.go b/walk_test.go index 9ebb059..7076bf6 100644 --- a/walk_test.go +++ b/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,18 @@ func TestWalkMapper(t *testing.T) { t.Errorf("statements = %q; want %q", walker.statements, wantStatements) } } + +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) + 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) + } +} From e6ce13f941138e6627c331ac0f809e4ca0653bb3 Mon Sep 17 00:00:00 2001 From: Noel Cower Date: Fri, 28 Sep 2018 15:19:10 -0700 Subject: [PATCH 2/3] Only use Map() node result if no error is returned This fixes #12 by only using the Node result of a Map() call if the Map() call was successful. Otherwise, if there's an error, it uses the original node to describe the error. In addition, it skips over nil children -- because a nil child cannot be mapped, it simply ignores them instead of modifying their cells. --- walk.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/walk.go b/walk.go index 79c6872..1348e53 100644 --- a/walk.go +++ b/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,16 +59,22 @@ 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] @@ -70,7 +82,7 @@ func walkInContext(context, parent ParentNode, walker Walker) (err error) { i-- continue } - children[i] = child + children[i] = newChild } switch child := child.(type) { From dd252e3be82d9a071006ebd7546ec3fdb55e0838 Mon Sep 17 00:00:00 2001 From: Noel Cower Date: Fri, 28 Sep 2018 15:41:26 -0700 Subject: [PATCH 3/3] Add an additional test around Map and nil nodes This tests both deleting an entire document's children and skipping over nil children in a document. --- walk_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/walk_test.go b/walk_test.go index 7076bf6..c1e4d53 100644 --- a/walk_test.go +++ b/walk_test.go @@ -326,6 +326,22 @@ func TestWalkMapper(t *testing.T) { } } +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 {}` @@ -333,6 +349,8 @@ func TestMapError(t *testing.T) { 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 }))