Elaborate on API philosophy

[JaredReisinger]

(Related to #119, #31, and maybe #32.)

I'd love to contribute to this project (for the team-related APIs in particular), but it feels like there's a change in the overall design philosophy, and it's not quite elaborated anywhere. In particular, I have questions in a couple of areas:

When/where do specific URLs get used?

There's some inconsistency as to whether URLs should be passed to the "service creator", with One() and All() taking no parameters, or the inverse with no parameters to the service creator, and Hyperlink/param arguments on One() and All(). As a case in point (return values omitted for clarity):

// users...
func (c *Client) Users(url *url.URL) {}  // returns *UsersService
func (u *UsersService) One() {}
func (u *UsersService) All() {}

// repositories
func (c *Client) Repositories() {}   // returns *RepositoriesService
func (r *RepositoriesService) One(uri *Hyperlink, params M) {}
func (r *RepositoriesService) All(uri *Hyperlink, params M) {}

I suspect this simply represents a transition in philosophy: (a) is the service pre-seeded or not, and (b) does it take a URL or Hyperlink/param? Regardless, only one of these patterns should be used, and it ought to be consistent across the package.

Where should multi-resource URLs live? Where should wrapper implementations live?

One of the Team-related APIs is "get a list of team members", which returns a []User. Should the URL live in the users.go file, or in teams.go (which I'm working on)? From the viewpoint of "it returns []User, and is just parameterized URL which eventually gets to Users().All(), you might say it belongs in users.go. But this conflicts with the structure of the GitHub documentation, and of the "natural" feel that it's a Team-related API that "just happens" to return users. If the project is also moving towards a friendlier style of API wrappers (like Teams().GetMembers(nil, M{"id": 123})), then since the wrapper likely lives in the "calling" service, the URL probably should as well.

What about pagination?

Every service seems to have an All() method, but it really only returns one page of results, and doesn't navigate/enumerate the result.NextPage links. I can kind of understand why the methods that take a URL have to be a bit agnostic about this, but it seems like there's a significant use-case for "I want all of the chained repos/users/teams starting from a given URL". In fact, that was my first expectation when I saw the All() methods; it wasn't until I looked at the implementation that I understood they really only return one page of data, not "all" of it. (One might argue that instead of One/All, it should be One/Some/All or One/Page/All where Some() (or Page()) returns a single page of data, and All() accumulates by following the NextPage links.)

A more fluent-style API?

From a programmer-usability standpoint, it seems a little weird to delay knowledge of parameters to the very end of the chain. In the team/members example, if I'm performing several operations on a team, it feels weird to have to remember to pass all of the replacement values at the end. I'd rather be able to do something like:

myTeamId := 123
team := octokit.Teams(myTeamId)
members := team.GetMembers()
managesMyRepo, _ := team.CheckRepository("my-org/my-repo")
if !managesMyRepo {
    team.UpdateRepository("my-org/my-repo", "pull")
}

or even:

success, result := octokit.Teams(123).Repository("my-org/my-repo").Update("pull")

With API wrappers, this is pretty straightforward—GetMembers() and CheckRepository() have a *TeamService as the receiver, and this have access to its properties—but it does impact the way that other values are passed in; perhaps instead of a generic octokit.M, the wrapper functions should take explicit parameters. (I think this makes discoverability much easier for developers new to the API.) Note that the underlying URLs are never passed; they are completely encapsulated by the wrapper methods.

Should this be in CONTRIBUTING.md?

Answers to these questions could live in README.md, but they are really targeted at folks wanting to contribute to the project, so perhaps it makes more sense to add them to CONTRIBUTING.md.

[pengwynn]

@JaredReisinger this is excellent. Sorry I'm just now circling back to follow up.

I suspect this simply represents a transition in philosophy

Yup. Passing the URL to .One() and friends is the new preferred way, to promote hypermedia and make it a little easier to work with GitHub Enterprise. This library was created minimally to power hub and we've been feeling out the interfaces we want as we go. We're still a long way from 1.0 (as you've seen from TODO.md). I'd like to hear your thoughts on which approach feels more natural to you.

Should the URL live in the users.go file, or in teams.go (which I'm working on)?
From the viewpoint of "it returns []User, and is just parameterized URL which eventually gets to Users().All(), you might say it belongs in users.go

Since path portion of the default URI template is hardcoded, I'd put the URL in the service that uses it, even though it might have the same URI parameters as templates in other services. So in this case, putting it in the Teams service makes sense to me.

(One might argue that instead of One/All, it should be One/Some/All or One/Page/All where Some() (or Page()) returns a single page of data, and All() accumulates by following the NextPage links.

Til now, we've put the pagination burden on the consumer. I'm intrigued by Some() or Page() as well as NextPage(). To truly get all resources, the client would need to be aware of rate limits. In the Ruby version of Octokit, we allow opting into auto-pagination.

A more fluent-style API?

You've aroused my curiosity.

team := octokit.Teams(myTeamId)

Would team here be a Team struct? We've typically treated these as value objects that don't have references to services or state. I'd be willing to discuss the pros and cons of your approach if you wanted to spike it out for a small part of the API.

Answers to these questions could live in README.md, but they are really targeted at folks wanting to contribute to the project, so perhaps it makes more sense to add them to CONTRIBUTING.md.

It might be worth having it in both places, with descriptive language in the README.md and prescriptive language in the CONTRIBUTING.md.

[Korede-TA]

Looks like this library hasn't received all too much love recently.

The questions about the overall design philosophy are indeed intriguing because the current API is a bit unwieldy to grasp.

I'd be more than willing to suggest alternative an alternative API topology to this that might be a bit easier to grasp and more compatible with the polymorphism mechanics and related features that Go offers.

I'll probably make a fork of this repository and refer back to this issue thread with updates.