Merge branch 'issue-37-cyclic-dep'

cyclic.go

@@ -1,29 +1,34 @@
 package task
 
-// HasCyclicDep checks if a task tree has any cyclic dependency
+// CheckCyclicDep checks if a task tree has any cyclic dependency
-func (e *Executor) HasCyclicDep() bool {
+func (e *Executor) CheckCyclicDep() error {
 	visits := make(map[string]struct{}, len(e.Tasks))
 
-	var checkCyclicDep func(string, *Task) bool
+	var checkCyclicDep func(string, *Task) error
-	checkCyclicDep = func(name string, t *Task) bool {
+	checkCyclicDep = func(name string, t *Task) error {
 		if _, ok := visits[name]; ok {
-			return false
+			return ErrCyclicDepDetected
 		}
 		visits[name] = struct{}{}
 		defer delete(visits, name)
 
 		for _, d := range t.Deps {
-			if !checkCyclicDep(d.Task, e.Tasks[d.Task]) {
+			// FIXME: ignoring by now. should return an error instead?
-				return false
+			task, ok := e.Tasks[d.Task]
+			if !ok {
+				continue
+			}
+			if err := checkCyclicDep(d.Task, task); err != nil {
+				return err
 			}
 		}
-		return true
+		return nil
 	}
 
 	for k, v := range e.Tasks {
-		if !checkCyclicDep(k, v) {
+		if err := checkCyclicDep(k, v); err != nil {
-			return true
+			return err
 		}
 	}
-	return false
+	return nil
 }

cyclic_test.go

@@ -4,6 +4,8 @@ import (
 	"testing"
 
 	"github.com/go-task/task"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestCyclicDepCheck(t *testing.T) {
@@ -18,9 +20,7 @@ func TestCyclicDepCheck(t *testing.T) {
 		},
 	}
 
-	if !isCyclic.HasCyclicDep() {
+	assert.Equal(t, task.ErrCyclicDepDetected, isCyclic.CheckCyclicDep(), "task should be cyclic")
-		t.Error("Task should be cyclic")
-	}
 
 	isNotCyclic := &task.Executor{
 		Tasks: task.Tasks{
@@ -34,7 +34,23 @@ func TestCyclicDepCheck(t *testing.T) {
 		},
 	}
 
-	if isNotCyclic.HasCyclicDep() {
+	assert.NoError(t, isNotCyclic.CheckCyclicDep())
-		t.Error("Task should not be cyclic")
+
+	inexixtentTask := &task.Executor{
+		Tasks: task.Tasks{
+			"task-a": &task.Task{
+				Deps: []*task.Dep{&task.Dep{Task: "invalid-task"}},
+			},
+		},
 	}
+
+	// FIXME: by now Task should ignore non existent tasks
+	// in the future we should improve the detection of
+	// tasks called with interpolation?
+	//     task:
+	//       deps:
+	//         - task: "task{{.VARIABLE}}"
+	//       vars:
+	//         VARIABLE: something
+	assert.NoError(t, inexixtentTask.CheckCyclicDep())
 }

errors.go

@@ -6,8 +6,8 @@ import (
 )
 
 var (
-	// ErrCyclicDependencyDetected is returned when a cyclic dependency was found in the Taskfile
+	// ErrCyclicDepDetected is returned when a cyclic dependency was found in the Taskfile
-	ErrCyclicDependencyDetected = errors.New("task: cyclic dependency detected")
+	ErrCyclicDepDetected = errors.New("task: cyclic dependency detected")
 	// ErrTaskfileAlreadyExists is returned on creating a Taskfile if one already exists
 	ErrTaskfileAlreadyExists = errors.New("task: A Taskfile already exists")
 )

task.go

@@ -61,8 +61,8 @@ type Task struct {
 
 // Run runs Task
 func (e *Executor) Run(args ...string) error {
-	if e.HasCyclicDep() {
+	if err := e.CheckCyclicDep(); err != nil {
-		return ErrCyclicDependencyDetected
+		return err
 	}
 
 	if e.Stdin == nil {