initial feedback about go-gh

[samcoe]

@vilmibm Thanks for the feedback!

I like the idea of including a DefaultRESTClient and DefaultGQLClient that will auto-resolve host and token. I don't think we would want to auto-resolve the remote as part of the client instantiation, just because the remote doesn't necessarily need to be resolved depending on the API endpoint being hit. I think it would be more useful to have a function called CurrentRepo that would do the resolving. In fact I am wondering if we should even expose the rest of the packages? @mislav mentioned offline that his main feedback would be to reduce the surface area exposed so we have less to maintain.

After thinking about it a bit more I wonder if we could get away with exposing only four functions in this module.

gh.Exec(args) -- to execute some gh command and return results
gh.DefaultRESTClient(opts) -- REST API client that auto-resolves host and token
gh.DefaultGQLClient(opts) -- GQL API client that auto-resolves host and token
gh.CurrentRepo() -- Resolves remotes to a current repository

This would mean moving config, git, auth packages to internal so that they can't be imported. I envision the api package still having some exposed pieces to give some basic configuration for the API clients.

What do you think?

[vilmibm]

Makes sense re: remotes and I like the gh.CurrentRepo() idea to start with.

For now, I'm definitely in favor of a consolidation like this. I'd like us to expose loading config since I'd like extensions to be able to store config stanzas, but I'm comfortable saving that for a future improvement as we might want to do more up front design on that interface.

Is the idea that the clients would resolve the token based on a provided host?

[samcoe]

Yeah that was what I was thinking. The client initializer options would allow specifying host and/or token, and if they are provided then skip the auto-resolving step for either.

[vilmibm]

👍 maybe we don't need to call them Default then?

[samcoe]

@vilmibm That makes sense. I made the changes if you would like to take another look.

[samcoe]

@vilmibm Going to close this issue out as I think I have addressed all the feedback here. Let me know if you think otherwise.