Export git package and git client #75
Conversation
samcoe
force-pushed
the
git-pkg
branch
2 times, most recently
from
1458ed4
to
5baaea0
Compare
pkg/git/client.go
Outdated
| } | ||
|
|
||
| type ClientOptions struct { | ||
| Command func(ctx context.Context, args ...string) (*exec.Cmd, error) |
There was a problem hiding this comment.
I don't love exposing something that is necessary for testing purposes only but couldn't think of a better way to go about this.
There was a problem hiding this comment.
This is a great start; thank you for spiking this out!
While this evolves, wouldn't it be better to first live in the cli/cli repo so that it can be immediately used by the code there? I had felt that the CLI codebase had more immediate use of this functionality than extensions do.
| Stderr io.Writer // will use stderr if none specified | ||
| Stdin io.Reader // will use stdin if none specified | ||
| Stdout io.Writer // will use stdout if none specified |
There was a problem hiding this comment.
Stdin, Stderr, and Stdout feel to me like they should be specified per each git operation, rather than as a property of the client.
Consider that a client could be used to call multiple operations:
client := git.NewClient()
client.Commit(message)
client.Push(remote, branch)If the same stdin was passed to both git invocations that read from it, it would have failed for the 2nd invocation because the 1st one would have closed it after reading from it. Furthermore, we might not want to connect the same stdout/stderr to every git operation.
There was a problem hiding this comment.
You are correct in that the second invocation will be broken but that is easy enough to fix with cmd.StdinPipe.
Having each method take in stdin, stdout, stderr, seems like a lot of arguments. It is very explicit though. I do imagine people would actually run into the same stdin being closed issue on subsequent method calls in this case also, since not everyone would realize it gets closed by cmd.Run().
I am envisioning that most methods will actually overwrite stdin, stdout, and stderr (that is what is happening in Remotes()) but the most basic and widely used case would be to just execute a command with all three attached to the exec.Cmd.
| } | ||
|
|
||
| type ClientOptions struct { | ||
| Command func(ctx context.Context, name string, args ...string) *exec.Cmd |
There was a problem hiding this comment.
We don't need to expose this publicly. Tests that need to stub out Client.command can just initialize the struct directly instead of using the constructor.
There was a problem hiding this comment.
You are correct, but what about tests outside of this package? I was thinking about tests being written in gh would probably need to utilize it.
There was a problem hiding this comment.
Oh, I didn't think at all about tests from outside of the package. But it feels wrong to me that the consumer of go-gh library (even if it's us) would need a mechanism to test go-gh internals. I feel that other codebases using this package should stub out git.Client invocations not by stubbing underlying git calls, but by avoiding calling real git.Client methods altogether.
| WorkTree string // will be passed to commands as --work-tree | ||
| } | ||
|
|
||
| func NewClient(opts *ClientOptions) Client { |
There was a problem hiding this comment.
If passing nil options is a valid way to initialize a zero-value Client, then couldn't we make the zero-value Client{} struct be useful as a client, and have users do that when there are no options?
And then, if there are some options, users can use NewClient() where the options struct would now be a required argument. Just a thought!
| } | ||
| } | ||
|
|
||
| func (c *Client) Command(ctx context.Context, args ...string) (*exec.Cmd, error) { |
There was a problem hiding this comment.
My advice would be to not make this public, otherwise the end caller has to be concerned with API details of git on the command-line, including the names of flags, etc. Ideally, we would have completely hidden git's own API behind our own Go API that just exposes the pieces of functionality that we need for our own purposes.
There was a problem hiding this comment.
Without having this exported then the only method the client has is Remotes. I feel like this method allows for ultimate flexibility when interacting with git and then we don't need to anticipate or implement every one of our users needs, we can just tell them to utilize Command and everything will be taken care of them. Going down the route of implementing every git command with every option they allow seems like a lot of work, and there are other git packages out there that do that which extension authors can choose to use instead if they like that interface.
|
@mislav Thanks for the feedback! You have raised some good points. It is possible that this work should be started in |
|
Offline, we have decided now is not the right time to introduce this feature into |
This PR exports the
gitpackage that had been internal for some time, no behavior changes here. It also adds in a newgitclient type that will be used for interacting with a localgitrepository. It is heavily inspired by this work done by@mislav. Right now the only exported methods on thegitclient areCommandwhich is the base method, similar toDoforhttp.Client, that all other methods will use to execute commands andRemoteswhich lists remotes for the local repo. The reason I addedRemotesis because it is needed withingo-gh. I think for the initial version these are the only two that will be supported but more will be added in the future as necessary.TODO: