Implemented user search

octokit / go-octokit

Simple Go wrapper for the GitHub API

https://github.com/octokit/go-octokit

MIT License

258 stars 79 forks source link

Implemented user search #77

Closed dhruvsinghal closed 9 years ago

dhruvsinghal commented 9 years ago

@pengwynn I have implemented the user search. Please have a look.

dhruvsinghal commented 9 years ago

@pengwynn PTAL.

Also, I'm just curious as to why the service is initialized to a single URL. Won't that prevent the service from being reused? Also, why is it the responsibility of the user code to fill in the placeholders in the URL? Shouldn't it be parameters to the respective service calls (like UserSearch, CodeSearch, etc.)?

pengwynn commented 9 years ago

Also, I'm just curious as to why the service is initialized to a single URL. Won't that prevent the service from being reused?

@dhruvsinghal It's a tradeoff. By accepting URLs, the client promotes passing around Hypermedia link relations, like those we get from the API root.

Shouldn't it be parameters to the respective service calls (like UserSearch, CodeSearch, etc.)?

I think we've got the same params for all four search flavors: q, page, sort, and order. Those could be named parameters or a SearchParameters struct I suppose.

pengwynn commented 9 years ago

@dhruvsinghal This is looking good. I'm tempted to implement another search type on this same branch before merging, just to see how our pattern will emerge.

@jingweno: any feedback?

dhruvsinghal commented 9 years ago

@pengwynn Sure, I'll implement another search, or if you want I can implement all the four searches on this branch itself.

dhruvsinghal commented 9 years ago

@pengwynn PTAL. I've implemented issue search as well.

dhruvsinghal commented 9 years ago

@pengwynn @jingweno, I propose the following restructuring of the SearchService Go API:

Given a Client object, obtain a SearchService object by calling Search()
Now, depending on the type of search (user, code, issues, or repositories) call the appropriate method e.g. UserSearch on the SearchService object.
The specific search object will take in the query, order and sort parameters (or nil if either of the latter two is not specified). This function will be responsible for expanding the URL that is defined at the top of this package.

Does thais sound reasonable?

owenthereal commented 9 years ago

In general it looks good except for some minor issues. Cheers!

dhruvsinghal commented 9 years ago

@jingweno Thanks for your comments!

@jingweno and @pengwynn PTAL.

owenthereal commented 9 years ago

:+1: :shipit:

dhruvsinghal commented 9 years ago

@jingweno Should I merge this branch into master?

pengwynn commented 9 years ago

Gonna go ahead and merge this since it follows the same pattern as the other API methods.

@jingweno I'd like to explore how we can better support query params in methods like this. /cc https://github.com/octokit/go-octokit/issues/72

owenthereal commented 9 years ago

@pengwynn

I'd like to explore how we can better support query params in methods like this. /cc #72

Yeah, this is a miss in the current design. Let's chat more about it in #72.