initial feedback

[vilmibm]

suggestions for the example:

• Include use of graphql
• Include the gh external call fallback

Regarding the package API:

• Is it worth including a shortcut way to get a REST/GQL client that auto-resolves host/token/remote based on the kind of logic we do in gh itself? As opposed to always requiring the three step process (pick remote, determine host, get+set token) to create clients?

Misc:

• I agree that inline docs with examples for godoc to pick up is A+ like we talked about in sync in addition to a full example
• something I've seen is running a full example as part of some build automation to make sure that it stays up to date. I'm not sure if that's too much effort here but it might be worth considering.

[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.