Alternative method signature for Exec / Simplified Error handling

[breml]

Hi,

I became aware of this by the Golang Weekly newsletter and I like the idea of goyek. I started to play around with it and I would like to start a discussion about two potential changes:

Signature change for goyek.Exec

Change the signature for goyek.Exec from

func Exec(name string, args ...string) func(*TF)

to

func Exec(cmdLine string) func(*TF)

This is inspired by github.com/bitfield/script (see https://github.com/bitfield/script/blob/9847d19e69d72d73b191e1f3c65d5dba94605a1c/filters.go#L123).

The main advantage of this change to me is, that it increases readability and it simplifies the process of copying back and forth of the shell commands between goyek and an actual shell (or a Makefile/Shell script one is migrating).
By using the backticks for the string in Go, in most cases there would be no need to modify the command while copying back and forth.

Error handling

I could imagine, that a task often has a pattern like this:

res, err := someFunc()
if err != nil {
	tf.Error(err)
}
...

Since goyek is somewhat inspired by testing.T (and its relatives), I though about how I nowadays handle errors in tests. I highly prefere the very simple https://github.com/matryer/is package, which provides me a method for error handling like this (https://pkg.go.dev/github.com/matryer/is#I.NoErr):

is.NoErr(err)

If the error is nil, the normal flow continues. If the error is non nil, the tests are ended with t.Fail().

For goyek I could imagine two methods with the following signature:

func (*goyek.TF).IfErr(err error, args ...interface{})
func (*goyek.TF).IfErrf(err error, format string, args ...interface{})

For the exact naming there are several options:

• tf.IfErr
• tf.IfError
• tf.IsErr
• tf.IsNoErr
• ...

Thanks again for your initiative, keep up the good work.

[pellared]

Hello @breml 👋

First of all, I would like to thank you a lot for taking your time to share feedback. It is invaluable. 🥇

Signature change for goyek.Exec

I totally understand your point. Especially, because I that even goyek's build pipeline has the following line (notice strings.Split):

goyek/build/build.go

Line 74 in f0ca8a1

tf.Cmd("gofumports", strings.Split("-l -w -local github.com/goyek/goyek .", " ")...).Run() //nolint // it is OK if it returns error

There are a few reasons why goyek does not implement the arg's splitting. I will do my best to elaborate on this (it will be a little chaotic because the problem is not easy for me).

1. For POSIX the programs accept []string args. See here. The shell is spriting the input into an array or args. AFAIK different shells do it a little differently.
2. For Windows the program accepts just a single command line string. See here. Because of that Go's exec.Cmd package does escaping and then joins the strings when executing the syscall. It uses https://msdn.microsoft.com/en-us/library/ms880421 for escaping but there a program for Windows which are working differently like msiexec - the main Windows program for installing software using MSI. Here is a sample workaround that is required to execute msiexec from Go: https://golang.org/pkg/os/exec/#Cmd
3. There are a lot of packages which does the args splitting and even more (e.g. env var interpolation) like https://github.com/mattn/go-shellwords , https://bitbucket.org/creachadair/shell/src/master/ , https://github.com/mvdan/sh . I was using something similar in the past and it occurred that we had some issues and we felt it was safer to rely on Go's []string approach. Implementing and maintaining something would be too much. Also, I prefer to not have any dependencies in goyek - I feel this is a benefit that currently it uses only the standard library.

I would rather prefer that this problem is tackled by the goyek's user e.g. by doing simple wrappers like:

package main

import (
	"fmt"
	"os/exec"

	"github.com/goyek/goyek"
	"github.com/mattn/go-shellwords"
)

func main() {
	flow := &goyek.Taskflow{}

	flow.Register(goyek.Task{
		Name:    "test",
		Usage:   "go test",
		Command: Exec("go test"),
	})

	flow.Register(goyek.Task{
		Name:  "lint",
		Usage: "golangci-lint",
		Command: func(tf *goyek.TF) {
			if err := Cmd(tf, "go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.39.0").Run(); err != nil {
				tf.Fatalf("install golangci-lint: %v", err)
			}
			if err := Cmd(tf, "golangci-lint run").Run(); err != nil {
				tf.Fatal(err)
			}
		},
	})

	flow.Main()
}

func Cmd(tf *goyek.TF, cmdLine string) *exec.Cmd {
	args, err := shellwords.Parse(cmdLine)
	if err != nil {
		tf.Fatalf("parse command line: %v", err)
	}
	return tf.Cmd(args[0], args[1:]...)
}

func Exec(cmdLine string) func(tf *goyek.TF) {
	args, err := shellwords.Parse(cmdLine)
	if err != nil {
		panic(fmt.Sprintf("parse command line: %v", err))
	}
	return goyek.Exec(args[0], args[1:]...)
}

Maybe it will be good to mention such possibility and design decision in https://github.com/goyek/goyek#helpers-for-running-programs.

What do you think?

Error handling

Just use https://github.com/stretchr/testify and pass tf instead of t, it should work 😉

There is a number of people preffering the more verbose standard library approach.

[breml]

@pellared Thanks for your detailed response. I completely understand your arguments regarding args handling in different OS and that it is the most reliable approach to rely on the stdlib for handling this correctly. I will certainly have a look at the packages you listed for arg splitting, because I still feel, there is some value in being able to quickly copy a command and run it in the shell (maybe with an additional argument). And I feel the readability is better especially if one has a good understanding of shell scripts and Makefiles. I think, I can life with the approach you presented in your answer. (If you go with the removal of Exec, as proposed in #62, it might be worth it to add the example above to the documentation as well as to your examples.)

I really like the way goyek lends to the implicit interface exposed by testing.T. I now get it how it works. Unfortunately, the package I normally use in tests (github.com/matryer/is) only implements parts of the testing.T signature (especially the Fail and FailNow) but not the Log and Logf part. So I will have to think about how to use this.

[pellared]

@breml Thanks for response. You make also want to take a look at this PR: fluentassert/verify#2, where I want use go-shellwords.

[pellared]

If you go with the removal of Exec, as proposed in #62, it might be worth it to add the example above to the documentation as well as to your examples.

Will do 👍 I just need some time 😉

Unfortunately, the package I normally use in tests (github.com/matryer/is) only implements parts of the testing.T signature (especially the Fail and FailNow) but not the Log and Logf part.

I think it won't be an issue. If something fails it will be logged to output anyway. And by default the same output is being used:

• https://github.com/matryer/is/blob/86ebe5e565bcc7b5a43fe7a5f62d13c281016c6f/is.go#L87

• goyek/taskflow.go

  Line 21 in f0ca8a1

  var DefaultOutput io.Writer = os.Stdout

Just use it 😉

[pellared]

@breml You can take a look at the the doc changes PR: #76

[pellared]

@breml I made matryer/is#42 so that you could use matryer/is.

I also consider making some kind of goyek/util repository. it could have cmd package that would contain a function with following signature: func Run(cmd string, opts ...Option).

PS. I encourage to check out the v2-rc of goyek 😉 I think it is pretty close to being stable...

[pellared]

@breml I created https://pkg.go.dev/github.com/goyek/x@v0.1.0/cmd

[breml]

@pellared Thanks for the ping. Unfortunately, I am not using goyek at the moment. But you never know, things can change.