-
Notifications
You must be signed in to change notification settings - Fork 22
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
Create an interface for executing Bazel commands #16
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.
LGTM, a few really minor thoughts
pkg/bazel.go
Outdated
|
||
type BazelCmdConfig struct { | ||
// Dir represents the working directory to use for the command. | ||
// If Dir is the empty string, use the calling process' current directory. |
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.
// If Dir is the empty string, use the calling process' current directory. | |
// If Dir is the empty string, use the calling process's current directory. |
pkg/bazel.go
Outdated
} | ||
|
||
type BazelCmd interface { | ||
Run(config BazelCmdConfig, args ...string) (int, error) |
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.
Possibly I'm being overly cautious here, but how would you feel about naming this Exec
or Execute
instead of Run
? I can imagine reading bazel.Run
as if it does a bazel run
(and having BazelCmd.{Build,Test,Run}(...)
rather than BazelCmd.Execute("build", ...)
would be a completely reasonable alternate API choice)
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 point, I'll change that.
pkg/bazel.go
Outdated
switch err.(type) { | ||
case *exec.ExitError: | ||
exitError, _ := err.(*exec.ExitError) | ||
return exitError.ExitCode(), err | ||
default: | ||
return -1, err | ||
} |
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.
switch err.(type) { | |
case *exec.ExitError: | |
exitError, _ := err.(*exec.ExitError) | |
return exitError.ExitCode(), err | |
default: | |
return -1, err | |
} | |
if exitError, ok := err.(*exec.ExitError); ok { | |
return exitError.ExitCode(), err | |
} else { | |
return -1, err | |
} |
Addressed comments |
This makes it easier to plug the Target Determinator into larger applications that already have tight control over how Bazel is executed (eg. when using this as a plugin of Aspect's CLI tool).
03f7af5
to
855ee0e
Compare
This makes it easier to plug the Target Determinator into larger
applications that already have tight control over how Bazel is executed
(eg. when using this as a plugin of Aspect's CLI tool).