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 across multiple repositories #835

Merged
merged 27 commits into from
Jul 24, 2015
Merged

search across multiple repositories #835

merged 27 commits into from
Jul 24, 2015

Conversation

shiftkey
Copy link
Member

Fixes #834

This is a breaking API change to support being able to search across repositories. We currently have an ctor overload that accepts an owner and name for the repository:

var request = new SearchIssuesRequest("windows", "aspnet", "dnx");

The change here is adding a public property Repos which accepts a collection of owner/name strings (i've added some validation to ensure these are formatted correctly for the server):

var request = new SearchIssuesRequest("windows");
request.Repos = new Collection<string> {
    "aspnet/dnx",
    "aspnet/dnvm"
};

This replaces the old Repo property, which was constrained to one string:

var request = new SearchIssuesRequest("windows");
request.Repo = "aspnet/dnx";

Rather than carry around both Repo and Repos for a period of time (leading to possible confusion), I'm just going to 🔥 the property here - I think many people would prefer to just use the Repos property anyway...

  • confirm regex rules match our repository rules
  • make changes in SearchIssueRequest
  • make changes in SearchCodeRequest
  • a more-friendly abstraction than just a collection?
  • document how to search issues (with some examples)
  • document how to search pull requests (with some examples)
  • [ ] document an example of searching code
  • wtf ci?

Rendered docs

cc @DamianEdwards @M-Zuber

@shiftkey shiftkey force-pushed the refine-search-api branch from 2c82a01 to b4139d4 Compare July 16, 2015 22:37
@shiftkey shiftkey force-pushed the refine-search-api branch from 23692df to d1bd50c Compare July 18, 2015 23:09
@shiftkey
Copy link
Member Author

@DamianEdwards @M-Zuber writing up a walkthrough of the search options available - how does this read?

@shiftkey shiftkey changed the title [WIP] search across multiple repositories search across multiple repositories Jul 18, 2015
@shiftkey shiftkey force-pushed the refine-search-api branch from 957e4b7 to fae9fc0 Compare July 19, 2015 02:29
@M-Zuber
Copy link
Contributor

M-Zuber commented Jul 19, 2015

It reads quite nicely.
The examples are very nice to have, a bonus would be a simple run down of the properties available even if they don't have examples. (I would be willing to try and do a PR once this in)

@khellang
Copy link
Contributor

What about creating a specialized object for holding the repos? This way you could use a custom collection initializer, forcing the user to specify both owner and repo name.

@shiftkey
Copy link
Member Author

@khellang good point - if I'm breaking the API here I'll see if I can do better than just a collection

@M-Zuber
Copy link
Contributor

M-Zuber commented Jul 19, 2015

Preaching to the choir, but I fully agree with @khellang

@khellang
Copy link
Contributor

Sent a PR at #842

@haacked
Copy link
Contributor

haacked commented Jul 24, 2015

This is looking pretty good. Anything needing specific review?

@shiftkey
Copy link
Member Author

@haacked not really, I should write an example for SearchCodeRequest.cs because that's impacted by this change. But I think everything in the code is ✨. I'll add a note.

@haacked
Copy link
Contributor

haacked commented Jul 24, 2015

@shiftkey should I merge this?

@shiftkey
Copy link
Member Author

@haacked sure, I just like putting docs in front of me - because otherwise they'll never get written 🚎

haacked added a commit that referenced this pull request Jul 24, 2015
search across multiple repositories
@haacked haacked merged commit 6aa323c into master Jul 24, 2015
@haacked haacked deleted the refine-search-api branch July 24, 2015 22:14
@haacked
Copy link
Contributor

haacked commented Jul 24, 2015

✨ all around.

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.

4 participants