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

Implement Pull Requests API #360

Merged
merged 44 commits into from
Feb 19, 2014
Merged

Implement Pull Requests API #360

merged 44 commits into from
Feb 19, 2014

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Feb 3, 2014

Hey @jpsullivan I'm going to finish this off as it's something I want to start using (and also the oldest PR).

Supersedes #173

Implements http://developer.github.com/v3/pulls/

  • tidy up outstanding feedback around documentation
  • integration test - create a pull request
  • integration test - update a pull request
  • integration test - close a pull request
  • integration test - merge a pull request
  • coding style
  • some more documentation 💟
  • bugfix - PR commits have author details - use different type to encapsulate this info

jpsullivan and others added 23 commits February 3, 2014 18:30
Still missing pull request files, commit lists and merge checks.
Still missing integration tests for Pull Requests, though.  I need to do
a bit more research before I can start to tackle that one.
Manually referenced as this requires Xamarin studio to do automatically.
Removed default parameter from pull request merge message according to
CA1026.
Bugfix for building the merge api url, as well.
Renamed and enhanced existing Merged tests.
- Initial integration test for pull requests (can't seem to do much
without the tree api)
- Added Observable unit tests for the rest of the pull request client
NewPullRequest constructor only takes required params.  Each required
parameter is now a readonly property.
@jpsullivan
Copy link
Contributor

No problem, man. Sorry I never ended up finishing it... We've been in crunch mode at the office for a while now and I really haven't had any time to give octokit any love :(

Thanks for all the help!

@shiftkey
Copy link
Member Author

dropped the [WIP] tag

Ready to review

return _connection.GetAndFlattenAllPages<Commit>(ApiUrls.PullRequestCommits(owner, name, number));
}
}
}

Choose a reason for hiding this comment

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

ZOMG! A missing trailing newline :trollface:

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey! I'm saving them for a rainy day! :trollface:

@nulltoken
Copy link

👍💣💥🔥

Strangely qed build 673 doesn't appear in the recent builds lists. Caching issue?

/cc @half-ogre


namespace Octokit
{
public class PullRequestCommit
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation? It's fine to do it later. :)

@haacked
Copy link
Contributor

haacked commented Feb 19, 2014

haacked added a commit that referenced this pull request Feb 19, 2014
@haacked haacked merged commit 4da4c39 into master Feb 19, 2014
@haacked haacked deleted the shiftkey/revive-pull-requests branch February 19, 2014 01:05
@haacked
Copy link
Contributor

haacked commented Feb 19, 2014

BTW, there's an alternate way to create a pull request using an issue number: http://developer.github.com/v3/pulls/#create-a-pull-request

It looks like this PR doesn't support that. Perhaps we can log another issue for that just to track it.

@shiftkey
Copy link
Member Author

@haacked you mean this? #372 :trollface:

@haacked
Copy link
Contributor

haacked commented Feb 19, 2014

Lo and behold! Once again, you are way ahead of me Mr. @shiftkey.

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