-
-
Notifications
You must be signed in to change notification settings - Fork 746
refactor: executor functional options #2085
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good refactor as always!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
executor.go
Outdated
func WithStdin(stdin io.Reader) ExecutorOption { | ||
return func(e *Executor) { | ||
e.Stdin = stdin | ||
} | ||
} | ||
|
||
func WithStdout(stdout io.Writer) ExecutorOption { | ||
return func(e *Executor) { | ||
e.Stdout = stdout | ||
} | ||
} | ||
|
||
func WithStderr(stderr io.Writer) ExecutorOption { | ||
return func(e *Executor) { | ||
e.Stderr = stderr | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory we could have an additional function that accepts all three, for shortness. Optional suggestion, though.
func WithIO(stdin io.Reader, stdout, stderr io.Writer) ExecutorOption {
return func(e *Executor) {
e.Stdin = stdin
e.Stdout = stdout
e.Stderr = stderr
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, it makes more sense to keep these separate if you're passing different values into each of them. Sometimes you might only want to pass in 1 or 2 of them and leave the others blank. However, I think if you're setting them all to the same value, we could have something like this:
func WithIO(rw io.ReadWriter) ExecutorOption {
return func(e *Executor) {
e.Stdin = rw
e.Stdout = rw
e.Stderr = rw
}
}
This would be super handy in the new tests I've been writing where we want to write everything to a single buffer.
ec2aa2e
to
60ff636
Compare
Couple of minor changes:
|
This PR migrates the
Executor
to use the functional options pattern. This follows other similar changes totaskfile.Reader
andtaskfile.Snippet
. This allows package API users to easily construct anExecutor
with sensible defaults and only supply a list of things they want to override.Our internal CLI implementation has a special functional option in
internal/flags
calledWithFlags
which will gather all the CLI flags and pass them in as functional options.