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

Add ApiOption overloads to IIssuesClient and IObservableIssuesClient #1274

Merged
merged 14 commits into from
Jun 2, 2016

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Apr 19, 2016

Fixes #1158

This is a big one, so I'm going through it as slowly and methodically as possible:

  • add new methods
  • update impacted unit tests
  • add new unit tests wherever appropriate
  • add new integration tests wherever appropriate
  • correct references in xml-docs

@shiftkey shiftkey changed the title [WIP] Add ApiOption overloads to IIssuesClient and IObservableIssuesClient [WIP] Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Apr 19, 2016
await Assert.ThrowsAsync<ArgumentNullException>(
() => client.GetAllForOwnedAndMemberRepositories(null, new ApiOptions()));
await Assert.ThrowsAsync<ArgumentNullException>(
() => client.GetAllForOwnedAndMemberRepositories(new IssueRequest(), null));
Copy link
Contributor

Choose a reason for hiding this comment

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

really minor nitpick but the equivalent code in other unit tests in this file are all on one line, while these ones are indented/split...

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 just my 80-chars-per-line OCD acting up. I'll undo this.

@ryangribble
Copy link
Contributor

it's hard to review these changes, but Im grasping at straws to find anything major here 😀

@shiftkey
Copy link
Member Author

@ryangribble thanks for the 👀, I think there's a bunch of new test coverage here I can do but given how many overloads there are I'm trying to keep this as focused as possible.

@shiftkey shiftkey force-pushed the update-issues-client branch from 77d722f to c9f4e54 Compare April 21, 2016 01:01
@shiftkey
Copy link
Member Author

I also took some time out in 1b2d009 to document the return values from the API. Let me know if you have any feedback on the words.

@shiftkey
Copy link
Member Author

Lots of the pagination endpoints here require you to have org access, so rather than getting too bogged down in pagination I'm going to skip those tests.

@shiftkey shiftkey changed the title [WIP] Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Apr 21, 2016
@ryangribble
Copy link
Contributor

I also took some time out in 1b2d009 to document the return values from the API. Let me know if you have any feedback on the words.

I noticed you are using <seealso cref="" when most other cases in the code base are using <see cref="" - is there any reason for using one over the other?

The wording sounds reasonable although I wonder whether the effort involved to doc all return values is worth it, since they are pretty self evident... and it then sorta means we have to go and add them to allll the rest of the codebase. I guess the same could be said for doccing most parameters in general though and if we didnt do it then we would have no doc anywhere. Does this mean you intend us to start the process of docing return value on everything from now on?

@shiftkey shiftkey changed the title Add ApiOption overloads to IIssuesClient and IObservableIssuesClient [WIP] Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Apr 29, 2016
@shiftkey shiftkey force-pushed the update-issues-client branch from cf76517 to ac01e40 Compare June 1, 2016 14:46
@shiftkey shiftkey changed the title [WIP] Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Add ApiOption overloads to IIssuesClient and IObservableIssuesClient Jun 1, 2016
@shiftkey
Copy link
Member Author

shiftkey commented Jun 1, 2016

Can I get a 👍 here before I close this out?

@shiftkey shiftkey merged commit dfebfe3 into octokit:master Jun 2, 2016
@shiftkey shiftkey deleted the update-issues-client branch June 2, 2016 09:21
@shiftkey shiftkey modified the milestone: Pagination Support Jun 3, 2016
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.

2 participants