Pull Requests API Implementation #173

jpsullivan · 2013-11-05T08:20:21Z

Attempting to take a stab at #135.

Basic functionality for Pull Requests is in place. As of now, the only things missing from this are:

Fetching commits within a pull request
Merging pull requests
Determining "mergibility" of a particular pull request
Integration tests for the PullRequestsClient class

Otherwise, to me this seems like a good starting point. Don't hold back if I screwed up something! 👍

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.

haacked · 2013-11-05T16:16:32Z

Octokit.Reactive/Clients/IObservablePullRequestsClient.cs

+        /// <param name="name">The name of the repository</param>
+        /// <param name="request">Used to filter and sort the list of pull requests returned</param>
+        /// <returns></returns>
+        IObservable<PullRequest> GetForRepository(string owner, string name, PullRequestRequest request);


PullRequestRequest hah! I guess that's a consequence of our current naming scheme. It's fiiiiine. I just had to comment as I LOL'd.

Ahaha, yeah I was wondering how you guys would react to that. I got a good laugh out of that as well :)

haacked · 2013-11-05T16:28:33Z

Thank you so much for this. This is great work. I had a few nitpicky comments.

I appreciate that you remembered to add the project files to the NetCor45 project. Most people (myself included) forget. Could you also add them to:

Unfortunately, I could not add these to the Solution so they're easy to miss.

jpsullivan · 2013-11-05T16:36:22Z

No problem! Happy to help :)

Already working on patching up those little things. As for the MonoTouch and MonoAndroid projects, I see them included within the solution, but as you can see they aren't "included" yet. I can certainly add the references manually, but I figured I'd check to make sure that something hasn't gotten screwed up with my development environment.

Thanks again for the great lib, you guys! Can't wait to contribute more 🤘

haacked · 2013-11-05T16:59:29Z

I think those project types only work in Xamarin Studio. Is that right @paulcbetts. So for now, manually editing them is our only recourse. Or installing Xamarin Studio and opening them in there.

Manually referenced as this requires Xamarin studio to do automatically.

jpsullivan · 2013-11-05T17:03:32Z

Ah, gotcha gotcha. That makes much more sense.

haacked · 2013-11-08T07:15:52Z

How's this coming along? Need any help? Looks like it's out of date with upstream master. Do you know how to update it?

jpsullivan · 2013-11-08T07:18:55Z

Sure do, just haven't gotten the time to dedicate to it these last couple days unfortunately. Planning on getting everything taken care of this weekend, though!

haacked · 2013-11-08T07:20:04Z

Great! Carry on! 🍰

anaisbetts · 2013-11-08T18:19:06Z

I think those project types only work in Xamarin Studio. Is that right @paulcbetts. So for now, manually editing them is our only recourse. Or installing Xamarin Studio and opening them in there.

They work in Visual Studio, but only if you have a Xamarin license which costs a fair amount of 💰. Don't worry about verifying the build or anything, just add in the files. If it works in Octokit.csproj it almost certainly works in the Mono version too.

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

jpsullivan · 2013-11-13T06:36:06Z

So I'm pretty sure that this is all taken care of, but it should be noted that integration tests can't really be done until the Trees API is implemented (unless I'm missing something here). I'm a little hesitant to call this "done" until those are implemented, but as far as I can tell, I've added in everything I can.

As always, feel free to let me know if I screwed up anything!

shiftkey · 2013-11-14T01:32:16Z

Octokit/Clients/PullRequestsClient.cs

+            Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
+            Ensure.ArgumentNotNullOrEmptyString(name, "name");
+
+            return ApiConnection.Get<PullRequestMerge>(ApiUrls.MergePullRequest(owner, name, number));


Merged doesn't return a response - you need to check the status code to see whether a pull request has been merged:

204 No Content indicates it has been merged
404 Not Found indicates it has not been merged

See this code for a similar example

NewPullRequest constructor only takes required params. Each required parameter is now a readonly property.

jpsullivan · 2013-11-27T17:54:48Z

Done and done. Loving that little utility to handle adding in all of the referenced .cs files! Very handy. 👍

shiftkey · 2013-11-27T18:02:27Z

@jpsullivan don't thank us, thank @forki! 😀

jpsullivan · 2013-11-27T18:04:41Z

@forki you're doing the Lords work, my son. 🍻

shiftkey · 2013-11-30T21:55:01Z

Octokit/Clients/RepositoriesClient.cs

@@ -204,4 +204,4 @@ public Task<string> GetReadmeHtml(string owner, string name)
        /// </remarks>
        public IRepoCollaboratorsClient RepoCollaborators { get; private set; }
    }
-}


Not sure what happened here, (merging master perhaps clobbering your changes?) but I've got a compiler error here:

Octokit\Clients\RepositoriesClient.cs(15,18,15,36): error CS0535: 'Octokit.RepositoriesClient' does not implement interface member 'Octokit.IRepositoriesClient.PullRequest'

shiftkey · 2013-11-30T22:55:21Z

@jpsullivan I've dropped a few comments in based on my testing and review. It's coming together!

jpsullivan · 2013-11-30T23:04:02Z

Thanks a lot man! Sorry that it seems kinda like amateur hour up in here... I'll get these patched up pronto.

shiftkey · 2013-11-30T23:21:56Z

@jpsullivan don't sweat it, take your time.

shiftkey · 2013-12-08T15:56:40Z

@jpsullivan anything left on this?

I'll review it the next time I have internet access but let me know if there's anything particular I should look at.

jpsullivan · 2013-12-08T16:06:17Z

@shiftkey Yep, still working on a few more details, particularly getting the integration tests sorted out now that I see Trees have been implemented. Will let you know when this is all ready for a review, though.

shiftkey · 2013-12-08T19:43:13Z

@jpsullivan ✨

nulltoken · 2014-01-27T14:46:26Z

Octokit.Reactive/Clients/IObservablePullRequestsClient.cs

+        IObservable<PullRequest> Create(string owner, string name, NewPullRequest newPullRequest);
+
+        /// <summary>
+        /// Creates a pull request for the specified repository. 


Shouldn't it be `Updates a pull request..``?

@nulltoken yes, yes it should

jpsullivan added 4 commits November 5, 2013 03:08

Stubbed out the API for pull requests.

47caffd

Still missing pull request files, commit lists and merge checks.

Stubbed out the reactive Pull Request functionality

fb04ad2

Tested out all of the existing Pull Request code

eebf101

Still missing integration tests for Pull Requests, though. I need to do a bit more research before I can start to tackle that one.

Fixed some goofed up test names

37f7de7

haacked reviewed Nov 5, 2013
View reviewed changes

Documented missing properties; Whitespace and typo tomfoolery patched

5f00bbe

Referenced pull request functionality for Mono projects

f671a47

Manually referenced as this requires Xamarin studio to do automatically.

jpsullivan added 9 commits November 12, 2013 00:17

Implemented pull request merge functionality

e8cef32

Merge comments default to an empty string as they are optional.

553c70d

Re-synced with upstream and patched merge conflicts

acbd798

Patched up some issues from the upstream merge.

5045291

Removed default parameter from pull request merge message according to CA1026.

Added tests for the Merge method

8036d12

Implemented Merged? functionality / tests

b2f05bf

Bugfix for building the merge api url, as well.

Unit test fixes

6357ebc

Pull request commit list now implemented / tested

5bc80e7

Renamed and enhanced existing Merged tests.

PullRequest model now contains all of necessary fields

1a1d93c

- 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

shiftkey reviewed Nov 14, 2013
View reviewed changes

NewPullRequest required parameters altered

bc446d1

NewPullRequest constructor only takes required params. Each required parameter is now a readonly property.

shiftkey reviewed Nov 30, 2013
View reviewed changes

jpsullivan added 3 commits December 7, 2013 19:52

Merge branch 'master' into jpsullivan/pull-requests-api

549135c

Fixed auto properties and moved integration tests around

16a1ae6

Killed with the hottest fire.

ac25d09

shiftkey mentioned this pull request Dec 8, 2013

WIP Implemented pull request review comments API #228

Closed

hnrkndrssn mentioned this pull request Jan 20, 2014

What's Left To Implement? #246

Closed

nulltoken mentioned this pull request Jan 27, 2014

First release SimonCropp/GitHubSync#1

Merged

2 tasks

nulltoken reviewed Jan 27, 2014
View reviewed changes

shiftkey mentioned this pull request Feb 3, 2014

Implement Pull Requests API #360

Merged

8 tasks

shiftkey closed this Feb 3, 2014

jpsullivan deleted the jpsullivan/pull-requests-api branch June 26, 2023 05:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull Requests API Implementation #173

Pull Requests API Implementation #173

jpsullivan commented Nov 5, 2013

haacked Nov 5, 2013

jpsullivan Nov 5, 2013

haacked commented Nov 5, 2013

jpsullivan commented Nov 5, 2013

haacked commented Nov 5, 2013

jpsullivan commented Nov 5, 2013

haacked commented Nov 8, 2013

jpsullivan commented Nov 8, 2013

haacked commented Nov 8, 2013

anaisbetts commented Nov 8, 2013

jpsullivan commented Nov 13, 2013

shiftkey Nov 14, 2013

shiftkey Nov 14, 2013

jpsullivan commented Nov 27, 2013

shiftkey commented Nov 27, 2013

jpsullivan commented Nov 27, 2013

shiftkey Nov 30, 2013

shiftkey commented Nov 30, 2013

jpsullivan commented Nov 30, 2013

shiftkey commented Nov 30, 2013

shiftkey commented Dec 8, 2013

jpsullivan commented Dec 8, 2013

shiftkey commented Dec 8, 2013

nulltoken Jan 27, 2014

shiftkey Jan 27, 2014

Pull Requests API Implementation #173

Pull Requests API Implementation #173

Conversation

jpsullivan commented Nov 5, 2013

haacked Nov 5, 2013

Choose a reason for hiding this comment

jpsullivan Nov 5, 2013

Choose a reason for hiding this comment

haacked commented Nov 5, 2013

jpsullivan commented Nov 5, 2013

haacked commented Nov 5, 2013

jpsullivan commented Nov 5, 2013

haacked commented Nov 8, 2013

jpsullivan commented Nov 8, 2013

haacked commented Nov 8, 2013

anaisbetts commented Nov 8, 2013

jpsullivan commented Nov 13, 2013

shiftkey Nov 14, 2013

Choose a reason for hiding this comment

shiftkey Nov 14, 2013

Choose a reason for hiding this comment

jpsullivan commented Nov 27, 2013

shiftkey commented Nov 27, 2013

jpsullivan commented Nov 27, 2013

shiftkey Nov 30, 2013

Choose a reason for hiding this comment

shiftkey commented Nov 30, 2013

jpsullivan commented Nov 30, 2013

shiftkey commented Nov 30, 2013

shiftkey commented Dec 8, 2013

jpsullivan commented Dec 8, 2013

shiftkey commented Dec 8, 2013

nulltoken Jan 27, 2014

Choose a reason for hiding this comment

shiftkey Jan 27, 2014

Choose a reason for hiding this comment