Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search Api Implmentation #226

Merged
merged 83 commits into from
Dec 24, 2013
Merged

Search Api Implmentation #226

merged 83 commits into from
Dec 24, 2013

Conversation

hahmed
Copy link
Contributor

@hahmed hahmed commented Nov 14, 2013

http://developer.github.com/v3/search/

I think we need to 💥 #203 (base was messed up).

Issue #137

Searching code issues repos and users.

I have implemented all the above with a basic search without any qualifiers added, however in this PR I have added all the qualifiers for searching repos. Below is a full list of what has been implemented for repo search.

Repo Qualifiers -> https://help.github.com/articles/searching-repositories

Note to self
To do: Any qualifiers related to code/users/issues search.
@shiftkey may want this to be in separate PR's, will need to confirm then update this.

This was referenced Nov 14, 2013
@hahmed
Copy link
Contributor Author

hahmed commented Nov 14, 2013

The only thing I need some review on is #203 (comment)

better conforms to the api, more simpler to implement too.
@hahmed
Copy link
Contributor Author

hahmed commented Nov 30, 2013

@haacked that's all in now and I have merged into master.

Let me know what else you want me to do to get this merged in.

Thanks

@shiftkey
Copy link
Member

@hahmed I'll give it a closer look this afternoon (will be in transit but online wherever possible)

@hahmed
Copy link
Contributor Author

hahmed commented Dec 1, 2013

@shiftkey cool let me know what you got I made some of those fixes anyway. With regards to optional ctor args [https://github.com//pull/226#commitcomment-4738657] @haacked mentioned to not do that here #226 (comment) [one of the diff comments - quoting some of it below].

Please don't suppress this one. For more details: http://haacked.com/archive/2010/08/10/versioning-issues-with-optional-arguments.aspx

The constructor should only have parameters that are required. The caller can set the properties of the object for all the optional fields.

If you want I can add some overloads but I don't think that will be of any benefit? Optional args are better but we are trying to avoid what @haacked said so the next best thing is to instantiate the Search Request with only the search term then customize your search however you want - we don't care.

Note - if we go down this route we will offer some consistency to our users with the 3 other search requests (code, issues and users).

I have one suggestion that will work sweetly - that would be to instantiate the object then maybe chain the other search terms.

var search = new SearchRepositoriesRequest("octoko")
                         .In(new[] { InQualifier.Description })
                         .Size(whatever)
                         .Language(Lanugage,Go);

If that can even be implemented like that?

Let me know what you are thinking with this...

@hahmed
Copy link
Contributor Author

hahmed commented Dec 8, 2013

@haacked @shiftkey do you guys feel this is good to merge? 😄

@shiftkey
Copy link
Member

shiftkey commented Dec 8, 2013

@hahmed I'll have a look at it tomorrow while I'm waiting for a flight

@hahmed
Copy link
Contributor Author

hahmed commented Dec 8, 2013

@shiftkey ok cool thanks.

Sorry if I did a bit too much work on this in one go - I should have broken it down into smaller steps by doing a simple search then gone on to extend the searching in more detail in simple digestible PR's. Lesson learnt though.

@haacked
Copy link
Contributor

haacked commented Dec 11, 2013

@hahmed looks like you'll need to update this branch from upstream master.

@hahmed
Copy link
Contributor Author

hahmed commented Dec 12, 2013

@hahmed looks like you'll need to update this branch from upstream master.

@haacked that's done.

@hahmed
Copy link
Contributor Author

hahmed commented Dec 20, 2013

@haacked @shiftkey if there is anything I can do to help push this along please let me know. I have updated this with the master branch now.

{
/// <summary>
/// search repos
/// http://developer.github.com/v3/search/#search-repositories
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: nevermind, I'd rather get this in and then tidy up the docs for the next release.

@shiftkey
Copy link
Member

Ok, I'm gonna land this one in master and open some issues to track tidying up things like

  • getting documentation more consistent
  • integration tests
  • code tidy-up and consistency

This will help us iterate over the search API as we charge towards V1.

Thanks for the hard work on this.

shiftkey added a commit that referenced this pull request Dec 24, 2013
@shiftkey shiftkey merged commit f495c70 into octokit:master Dec 24, 2013
@hahmed
Copy link
Contributor Author

hahmed commented Dec 24, 2013

@shiftkey it's cool, if you want. You can cc me in anything.

I have a few other things I need to do I will look to implement them in smaller portions hopefully. (if anyone else has not taken the issue).

@hahmed
Copy link
Contributor Author

hahmed commented Dec 24, 2013

@shiftkey can the branch hahmed/search-api be removed please - mistake on my part. Thanks

@hahmed hahmed deleted the hahmed/search-api branch December 24, 2013 21:11
@shiftkey
Copy link
Member

@hahmed I'm not clear on what you need me to do. I can't see the branch listed here so I assume you tidied it up:

https://github.com/hahmed/octokit.net/branches

@hahmed
Copy link
Contributor Author

hahmed commented Dec 25, 2013

@shiftkey sorry I should have been more clearer - I meant https://github.com/octokit/octokit.net/tree/hahmed/search-api

@shiftkey
Copy link
Member

Ah, yes. Don't stress about those, they're easy to clean up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants