Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task parameters #32

Merged
merged 7 commits into from
Jul 5, 2017
Merged

Add task parameters #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
196d3cb
add custom Cmd and Dep types
andreynering Jul 2, 2017
b8fe8d4
refactor: onyl read Taskvars file once
andreynering Jul 4, 2017
e1fc3aa
remove support for TOML and JSON
andreynering Jul 4, 2017
06d80e9
rename Cmd.Params to Cmd.Vars
andreynering Jul 5, 2017
313d708
fix lint
andreynering Jul 5, 2017
a52a66e
test usage of param with $ prefix
andreynering Jul 5, 2017
01b9bf5
update README documentation about calling another task
andreynering Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package task

import (
"errors"
"strings"
)

type Params map[string]string

type Cmd struct {
Cmd string
Task string
Params Params
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on what's easiest to document in the README.

If you need to rename "parms" -> "vars" for the end-user only, you could probably add a struct tag here and in dep, if your yaml lib supports it:

  Params Params `yaml:"vars"`

}

type Dep struct {
Task string
Params Params
}

var (
ErrCantUnmarshalCmd = errors.New("task: can't unmarshal cmd value")
ErrCantUnmarshalDep = errors.New("task: can't unmarshal dep value")
)

func (c *Cmd) UnmarshalYAML(unmarshal func(interface{}) error) error {
var cmd string
if err := unmarshal(&cmd); err == nil {
if strings.HasPrefix(cmd, "^") {
c.Task = strings.TrimPrefix(cmd, "^")
} else {
c.Cmd = cmd
}
return nil
}
var taskCall struct {
Task string
Params Params
}
if err := unmarshal(&taskCall); err == nil {
c.Task = taskCall.Task
c.Params = taskCall.Params
return nil
}
return ErrCantUnmarshalCmd
}

func (d *Dep) UnmarshalYAML(unmarshal func(interface{}) error) error {
var task string
if err := unmarshal(&task); err == nil {
d.Task = task
return nil
}
var taskCall struct {
Task string
Params Params
}
if err := unmarshal(&taskCall); err == nil {
d.Task = taskCall.Task
d.Params = taskCall.Params
return nil
}
return ErrCantUnmarshalDep
}
54 changes: 54 additions & 0 deletions command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package task_test

import (
"testing"

"github.com/go-task/task"

"github.com/stretchr/testify/assert"
"gopkg.in/yaml.v2"
)

func TestCmdParse(t *testing.T) {
const (
yamlCmd = `echo "a string command"`
yamlDep = `"task-name"`
yamlTaskCall = `
task: another-task
params:
PARAM1: VALUE1
PARAM2: VALUE2
`
)
tests := []struct {
content string
v interface{}
expected interface{}
}{
{
yamlCmd,
&task.Cmd{},
&task.Cmd{Cmd: `echo "a string command"`},
},
{
yamlTaskCall,
&task.Cmd{},
&task.Cmd{Task: "another-task", Params: task.Params{"PARAM1": "VALUE1", "PARAM2": "VALUE2"}},
},
{
yamlDep,
&task.Dep{},
&task.Dep{Task: "task-name"},
},
{
yamlTaskCall,
&task.Dep{},
&task.Dep{Task: "another-task", Params: task.Params{"PARAM1": "VALUE1", "PARAM2": "VALUE2"}},
},
}
for _, test := range tests {
err := yaml.Unmarshal([]byte(test.content), test.v)
assert.NoError(t, err)
assert.Equal(t, test.expected, test.v)
}
}
2 changes: 1 addition & 1 deletion cyclic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ func (e *Executor) HasCyclicDep() bool {
defer delete(visits, name)

for _, d := range t.Deps {
if !checkCyclicDep(d, e.Tasks[d]) {
if !checkCyclicDep(d.Task, e.Tasks[d.Task]) {
return false
}
}
Expand Down
8 changes: 4 additions & 4 deletions cyclic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ func TestCyclicDepCheck(t *testing.T) {
isCyclic := &task.Executor{
Tasks: task.Tasks{
"task-a": &task.Task{
Deps: []string{"task-b"},
Deps: []*task.Dep{&task.Dep{Task: "task-b"}},
},
"task-b": &task.Task{
Deps: []string{"task-a"},
Deps: []*task.Dep{&task.Dep{Task: "task-a"}},
},
},
}
Expand All @@ -25,10 +25,10 @@ func TestCyclicDepCheck(t *testing.T) {
isNotCyclic := &task.Executor{
Tasks: task.Tasks{
"task-a": &task.Task{
Deps: []string{"task-c"},
Deps: []*task.Dep{&task.Dep{Task: "task-c"}},
},
"task-b": &task.Task{
Deps: []string{"task-c"},
Deps: []*task.Dep{&task.Dep{Task: "task-c"}},
},
"task-c": &task.Task{},
},
Expand Down
81 changes: 41 additions & 40 deletions task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ type Tasks map[string]*Task

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless the semantics of a nil task is needed, or unless you often need to retrieve a task pointer, you could concider to use map[string]Task. It will have some consequences for how you can write to the tasks though, as this will then fetch out a copy:

t, ok := e.tasks[name]

// Task represents a task
type Task struct {
Cmds []string
Deps []string
Cmds []*Cmd
Deps []*Dep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is any case where it makes sense to have nil cmds/deps it's probably better to not use pointers here.

Why?

Code wise it's usually cleaner.

As an FYI: Memory allocation / performance wise, it's usually better to have all the actual structs within the slice, so that all the memory for these objects will be allocated in one big slice of bytes and are more likely to be accessed faster in for-loops due to pipelining. Also there is less memory fractions and less garbage-collection work to do. This only really matters in tight loops for huge programs though; it's not going to matter for task.

Desc string
Sources []string
Generates []string
Expand Down Expand Up @@ -83,26 +83,26 @@ func (e *Executor) Run(args ...string) error {
}

for _, a := range args {
if err := e.RunTask(context.Background(), a); err != nil {
if err := e.RunTask(context.Background(), a, nil); err != nil {
return err
}
}
return nil
}

// RunTask runs a task by its name
func (e *Executor) RunTask(ctx context.Context, name string) error {
func (e *Executor) RunTask(ctx context.Context, name string, params Params) error {
t, ok := e.Tasks[name]
if !ok {
return &taskNotFoundError{name}
}

if err := e.runDeps(ctx, name); err != nil {
if err := e.runDeps(ctx, name, params); err != nil {
return err
}

if !e.Force {
upToDate, err := e.isTaskUpToDate(ctx, name)
upToDate, err := e.isTaskUpToDate(ctx, name, params)
if err != nil {
return err
}
Expand All @@ -113,27 +113,27 @@ func (e *Executor) RunTask(ctx context.Context, name string) error {
}

for i := range t.Cmds {
if err := e.runCommand(ctx, name, i); err != nil {
if err := e.runCommand(ctx, name, i, params); err != nil {
return &taskRunError{name, err}
}
}
return nil
}

func (e *Executor) runDeps(ctx context.Context, task string) error {
func (e *Executor) runDeps(ctx context.Context, task string, params Params) error {
g, ctx := errgroup.WithContext(ctx)
t := e.Tasks[task]

for _, d := range t.Deps {
dep := d
d := d

g.Go(func() error {
dep, err := e.ReplaceVariables(task, dep)
dep, err := e.ReplaceVariables(d.Task, task, params)
if err != nil {
return err
}

if err = e.RunTask(ctx, dep); err != nil {
if err = e.RunTask(ctx, dep, d.Params); err != nil {
return err
}
return nil
Expand All @@ -146,28 +146,32 @@ func (e *Executor) runDeps(ctx context.Context, task string) error {
return nil
}

func (e *Executor) isTaskUpToDate(ctx context.Context, task string) (bool, error) {
func (e *Executor) isTaskUpToDate(ctx context.Context, task string, params Params) (bool, error) {
t := e.Tasks[task]

if len(t.Status) > 0 {
return e.isUpToDateStatus(ctx, task)
return e.isUpToDateStatus(ctx, task, params)
}
return e.isUpToDateTimestamp(ctx, task)
return e.isUpToDateTimestamp(ctx, task, params)
}

func (e *Executor) isUpToDateStatus(ctx context.Context, task string) (bool, error) {
func (e *Executor) isUpToDateStatus(ctx context.Context, task string, params Params) (bool, error) {
t := e.Tasks[task]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's considered clean code (by some author called Robert C. Martin) to try to keep the number of positional parameters in a function to a minimum. Often just 1, or maybe 2, or in worst case 3. Generally I think you here handle the trade-offs well, but I will give some tips that you could consider if you feel it's necessary, or otherwise feel free to ignore for now.

Since all the helper methods that need params, also needs the task name, one way of reducing the number of positional parameters, could be to use the Deps struct in place of task and params. It might require another type name to not seem confusing in this context, e.g. TaskReference, but that will work pretty well in the original struct where it's used as well (Deps []TaskReference). It could even be given a helper function that fetches the task (with Vars overwritten by params) from an Executor instance. This also lets you deal with the lookup-error handling once.

Something similar could be done for when you need to reference a Cmd, where you basically use the same struct but include and Index int for looking up

type TaskReference struct{
           Task string
           Params Params
}

// Task returns a copy of the task in e with Vars updated with values from Params,
// or an error if the task don't exist.
func (tr *TaskReference) Task(e *Executor) (*Task, error) {
}

type CmdReference struct{
           TaskReference
           Cmd int
}

// Cmd returns a copy of the command in e with all template variables replaces by
// intended values.
func (tr *CmdReference) Cmd(e *Executor) (*Cmd, error) {
}


environ, err := e.getEnviron(task)
environ, err := e.getEnviron(task, params)
if err != nil {
return false, err
}
dir, err := e.getTaskDir(task)
dir, err := e.getTaskDir(task, params)
if err != nil {
return false, err
}
status, err := e.ReplaceSliceVariables(t.Status, task, params)
if err != nil {
return false, err
}

for _, s := range t.Status {
for _, s := range status {
err = execext.RunCommand(&execext.RunCommandOptions{
Context: ctx,
Command: s,
Expand All @@ -181,23 +185,23 @@ func (e *Executor) isUpToDateStatus(ctx context.Context, task string) (bool, err
return true, nil
}

func (e *Executor) isUpToDateTimestamp(ctx context.Context, task string) (bool, error) {
func (e *Executor) isUpToDateTimestamp(ctx context.Context, task string, params Params) (bool, error) {
t := e.Tasks[task]

if len(t.Sources) == 0 || len(t.Generates) == 0 {
return false, nil
}

dir, err := e.getTaskDir(task)
dir, err := e.getTaskDir(task, params)
if err != nil {
return false, err
}

sources, err := e.ReplaceSliceVariables(task, t.Sources)
sources, err := e.ReplaceSliceVariables(t.Sources, task, params)
if err != nil {
return false, err
}
generates, err := e.ReplaceSliceVariables(task, t.Generates)
generates, err := e.ReplaceSliceVariables(t.Generates, task, params)
if err != nil {
return false, err
}
Expand All @@ -215,28 +219,25 @@ func (e *Executor) isUpToDateTimestamp(ctx context.Context, task string) (bool,
return generatesMinTime.After(sourcesMaxTime), nil
}

func (e *Executor) runCommand(ctx context.Context, task string, i int) error {
func (e *Executor) runCommand(ctx context.Context, task string, i int, params Params) error {
t := e.Tasks[task]
cmd := t.Cmds[i]

c, err := e.ReplaceVariables(task, t.Cmds[i])
if err != nil {
return err
if cmd.Cmd == "" {
return e.RunTask(ctx, cmd.Task, cmd.Params)
}

if strings.HasPrefix(c, "^") {
c = strings.TrimPrefix(c, "^")
if err = e.RunTask(ctx, c); err != nil {
return err
}
return nil
c, err := e.ReplaceVariables(cmd.Cmd, task, params)
if err != nil {
return err
}

dir, err := e.getTaskDir(task)
dir, err := e.getTaskDir(task, params)
if err != nil {
return err
}

envs, err := e.getEnviron(task)
envs, err := e.getEnviron(task, params)
if err != nil {
return err
}
Expand Down Expand Up @@ -266,22 +267,22 @@ func (e *Executor) runCommand(ctx context.Context, task string, i int) error {
return nil
}

func (e *Executor) getTaskDir(name string) (string, error) {
t := e.Tasks[name]
func (e *Executor) getTaskDir(task string, params Params) (string, error) {
t := e.Tasks[task]

exeDir, err := e.ReplaceVariables(name, e.Dir)
exeDir, err := e.ReplaceVariables(e.Dir, task, params)
if err != nil {
return "", err
}
taskDir, err := e.ReplaceVariables(name, t.Dir)
taskDir, err := e.ReplaceVariables(t.Dir, task, params)
if err != nil {
return "", err
}

return filepath.Join(exeDir, taskDir), nil
}

func (e *Executor) getEnviron(task string) ([]string, error) {
func (e *Executor) getEnviron(task string, params Params) ([]string, error) {
t := e.Tasks[task]

if t.Env == nil {
Expand All @@ -291,7 +292,7 @@ func (e *Executor) getEnviron(task string) ([]string, error) {
envs := os.Environ()

for k, v := range t.Env {
env, err := e.ReplaceVariables(task, fmt.Sprintf("%s=%s", k, v))
env, err := e.ReplaceVariables(fmt.Sprintf("%s=%s", k, v), task, params)
if err != nil {
return nil, err
}
Expand Down
Loading