Add task parameters

[andreynering]

Basic implementation of #31

Supported both in cmds and deps.

TODO:

• Support space in parameters. I'm thinking of using quotes for that.
  • ~^task PARAM="value with spaces"~~
  • In this case, it should also be possible to sanitize a quote: ^task PARAM="value with \"quotes\"". Any bettersimpler idea for this?
• Use custom YAML marshaler instead of any specific syntax
• Check if it's working well with $ prefix?
• Add documentation on README

[smyrman]

Support space in parameters. I'm thinking of using quotes for that.
^task PARAM="value with spaces"

Is there anyway we could leave YAML to handle it for us? E.g.:

taskA:
  cmds:
    - echo "I am a string"
    - {"task": "taskB", "PARAM": "value with spaces"}

A bit more readable perhaps, I assume this would be equivalent:

taskA:
  cmds:
    - echo "I am a string"
    - task: taskB
      PARAM: value with spaces

[smyrman]

I would suggest we do this even earlier, when we parse the YAML/JSON/TOML. This would allow us to let YAML/TOML/JSON handle quotes for parameter values, and thus confuse the user less. This would also allow us to deprecate the "^" syntax in favour of a more explicit one. For instance we could say we create a new type "instruction":

 type Task struct {		
 	Cmds      []instruction
 	Deps      []instruction
        ...
}

type instruction struct {
        IsShellCmd bool
        Task string  // holds name of task or full shell command.
        Vars map[string]string
}

func (instr *instruction) UnmarshalJSON(data []byte) error {
        m := make(map[string]string)
        err := json.Unmarshal(data, m)
        if err =! nil {
                instr.IsShellCmd = true
                return json.Unmarshal(data, &instr.Task)
        }
        instr.IsShellCmd = false
        instr.Task = m["task"]   // the task name, including an empty value (""), will be validated later.
        delete(m["task"])
        instr.Vars = m
        }
}

// Equivalent for YAML/TOML if needed... 

I don't know if YAML/TOML will "fall-back" to convert data to JSON and parse it to UnamrshalJSON if an UnmarshalJSON function is found? that would be crazy performance wise, but functional wise it might make sense if they did?

[smyrman]

Maybe even more explicit if we do this...

taskA:
  cmds:
    - echo "I am a string"
    - task: taskB
      vars:
        - PARAM1: I am a string with spaces
      env:
        - ENV1: |
          We could even override the env, but let's skip
          that for the first implementation not to complicate
          things to much.

But at same time the indentation here is starting to get very deep!

[andreynering]

@smyrman Yeah, since YAML is a very powerful markup language, I think I'll use more of it and less of any specific syntax.

I could write a custom marshaler for YAML and JSON, but not TOML. See #34

I just started the migration, but I'll need a bit more time to complete it.

[andreynering]

@smyrman Pushed. Using the more verbose version (with params: key) by now, but that is easy to change if we want. (In fact, it became very verbose).

Feedback would be awesome.

Task is in need of a few refactorings. Good that we have tests to ensure everything keep working while doing these changes.

[smyrman]

Looks great.

One question is if "params" is really the best name for usage in the YAML files, or if it should be called "vars" to give the end-user fewer terms to remember. A good test for what's the best thing to do would be to write the READEM documentation and see what name makes the documentation easiest to write/read.

I believe that if you find you need to use a lot of documentation real-estate to explain a feature, it's often designed or named slightly wrong. Likewise, if it becomes very easy to document, the design and naming is probably good.

[smyrman]

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.

[smyrman]

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]

[smyrman]

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"`

[smyrman]

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) {
}

[smyrman]

Don't be afraid to ignore any of my comments above btw. They are just comments. Your code is good.

[andreynering]

@smyrman I appreciate your comments! Some I already had in mind, like the TaskReference idea.

I think I'll change params to vars and this PR is done. I'll still do some refactorings after, though.

I'm still not sure about the final syntax. Any thought?

task:
  cmds:
    - task: task-name
      PARAM: my-param

vs.

task:
  cmds:
    - task: task-name
      vars:
        PARAM: my-param

The second one is more verbose, but also more future-proof (if we ever have more properties other than vars in the future).

[andreynering]

I think I'll keep the verbose version. One can use YAML magic to use less lines:

task:
  cmds:
    - task: task-name
      vars: {PARAM: my-param, ANOTHER_PARAM: value}

[smyrman]

I think the verbose syntax is good. It also opens up for overriding "env" in the future without compromising backwards-compatibility.