Base class for searching GitHub #301
Conversation
it's simpler if we join the qualifiers...
|
The reason for sort: All searches - Search/Repo, Search/Users, Search/Issues, Search/Files have sort added to the query string, so we can get inherited class to simply pass us the string representation of whatever Sort means to them. |
|
Sounds good to me! From: Haroon We are repeating a lot of logic inside the searches, hopefully this class will allow us to simply our searching. I will close hahmed#1 so we can keep our discussions on the octokit repo (if anyone else has any input). How do you want this to be merged? I have created the base class, so we can now iterate on it till it's right... Once this is merged - we can open a PR for every search to inherit from base class? @alfhenrik can then use it for his PR #290 and I can also use it for #289 cc:// @shiftkey @alfhenrik You can merge this Pull Request by running: git pull https://github.com/hahmed/octokit.net search-base-class Or you can view, comment on it, or merge it online at: -- Commit Summary --
-- File Changes -- -- Patch Links -- https://github.com/octokit/octokit.net/pull/301.patch Reply to this email directly or view it on GitHub: |
| /// <summary> | ||
| /// All qualifiers that are used for this search | ||
| /// </summary> | ||
| public abstract System.Collections.Generic.IReadOnlyCollection<string> MergedQualifiers(); |
There was a problem hiding this comment.
You could add System.Collections.Generic in a using statement together with the other usings at the top, makes the method a bit cleaner.
| @@ -56,6 +56,7 @@ | |||
| <Compile Include="Clients\ActivitiesClient.cs" /> | |||
| <Compile Include="Clients\SearchClient.cs" /> | |||
| <Compile Include="Clients\ISearchClient.cs" /> | |||
| <Compile Include="Models\Request\SearchBase.cs" /> | |||
There was a problem hiding this comment.
Shouldn't the file name be BaseSearchRequest.cs?
There was a problem hiding this comment.
@alfhenrik thanks! 👍
|
@hahmed it still says "WIP:" in the PR - is there anything left to implement on this? 👍 on this change, hopefully this tidies up a bunch of our boilerplate code |
|
Nope... forgot to remove that. Was not sure if you wanted to add anything else to this. |
|
@shiftkey removed the WIP... ready for review now. |
|
Given we already discussed a bunch of this over in hahmed#1, I don't have anything else really to find fault with. |
Base class for searching GitHub
http://developer.github.com/v3/search/#search
We are repeating a lot of logic inside the searches, hopefully this class will allow us to simply our searching.
I will close hahmed#1 so we can keep our discussions on the octokit repo (if anyone else has any input).
How do you want this to be merged?
I have created the base class, so we can now iterate on it till it's right...
Once this is merged - we can open a PR for every search to inherit from base class? @alfhenrik can then use it for his PR #290 and I can also use it for #289
cc:// @shiftkey @alfhenrik