Fork/Hook issue #698 #776

kristianhald · 2015-03-23T17:25:43Z

Hi,

First time doing a pull request, so if I have done anything different than expected, then I would be happy to know.

I have tried continuing on the pr for the issue #495, where it was stated that it was missing integration tests.

Therefore, I would like to have some input on the changes made and if I have missed anything, that needs to be changed.

I have done the following changes to the code:

Updated the previous code to match current convention rules and changes made in the interfaces
Implemented integration tests for the forks api
Implemented integration tests for the hooks api
Implemented hooks ping command
Added 'sort' parameter to forks list command
Merged master into the branch to get the code up to date as the original pull request was old

Tasks missing:

Waiting for response from GitHub on error reported, when creating a hook and stating it should be 'active' = false, then it is created as 'active' = true. Minor error, but I have a test checking it and I do not like having a test that creates active web hooks (I can change the assert, but as I would like some input on the code, there is some time)

…rated out - there doesn't need to be this duplicitous 'clarity' given the classes are now separate

…RepositoryHooksClient

…udio mono things

Conflicts: Octokit.Reactive/Clients/IObservableRepositoriesClient.cs Octokit.Reactive/Clients/ObservableRepositoriesClient.cs Octokit.Reactive/Octokit.Reactive-Mono.csproj Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj Octokit.Reactive/Octokit.Reactive-Monotouch.csproj Octokit.Reactive/Octokit.Reactive.csproj Octokit/Octokit-Mono.csproj Octokit/Octokit-Monotouch.csproj Octokit/Octokit-netcore45.csproj Octokit/Octokit.csproj

Conflicts: Octokit.Reactive/Clients/IObservableRepositoriesClient.cs Octokit.Reactive/Clients/ObservableRepositoriesClient.cs Octokit.Reactive/Octokit.Reactive-Mono.csproj Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj Octokit.Reactive/Octokit.Reactive-Monotouch.csproj Octokit.Tests/OctoKit.Tests-NetCore45.csproj Octokit/Helpers/ApiUrls.cs Octokit/Octokit-Mono.csproj Octokit/Octokit-Monotouch.csproj Octokit/Octokit-Portable.csproj Octokit/Octokit-netcore45.csproj

…nd code analysis.

… implementation of the forks api.

…lls.

…hooks.

Conflicts: Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj

kristianhald · 2015-04-20T22:57:49Z

Hi @shiftkey thanks for the comments on the code and the design. I have cleaned it up and added the Post method.
I have added to each of your comments a short description of the change I have made in the code and I have tried keeping the changes in their own commits, so that you can compare the changes.

The 'Changed await AssertEx.Throws' to 'Assert.ThrowsAsync' is wrong. I accidentally also removed the 'await'. They have been added again to the tests in a later commit.

I ran a bit out of time here at the last moment, so I hope everything is ok. Please write back in regards to any changes that needs to be added.

kristianhald · 2015-04-28T20:43:37Z

hi @shiftkey, Just wanted to make certain that the code is ok and not missing anything. I did the merge and solved the conflicts. I think that the merge from master must be done close to the merge to master. If the code changes are ok, I will do another merge.

kbilsted · 2015-05-04T20:48:13Z

What is the status of this PR? I really need the features it adds.

shiftkey · 2015-05-04T20:49:23Z

@kristianhald apologies for the delay - I've just gotten back from BUILD so am digging myself out of inbox hell. I'll add this to the list of things to review today. There's a merge conflict with the branch, but we can address that separate to the changes here.

kbilsted · 2015-05-04T21:07:39Z

@shiftkey Great stuff. As long as I know this is on your radar, I can wait a few more cycles :-)

shiftkey · 2015-05-08T06:27:12Z

Octokit.Tests/Clients/RepositoryForksClientTests.cs

+            {
+                var client = new RepositoriesClient(Substitute.For<IApiConnection>());
+
+                await Assert.ThrowsAsync<ArgumentNullException>(async () => await client.Forks.GetAll(null, "name", null));


💄 the inner async/await pair can be scrubbed from these files, i.e. () => client.Forks....

Gone @ 🔫 point. They wanted soo much to 💤

shiftkey · 2015-05-08T06:28:38Z

@kristianhald looking good, just a bit of cleanup for the tests and merging master in and I'm happy to take this.

shiftkey · 2015-05-08T06:29:39Z

Octokit.Tests.Integration/Clients/RepositoryHooksClientTests.cs

+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+using Octokit.Tests.Helpers;


🔥 unnecessary using statement

😔 Has been removed.

kristianhald · 2015-05-08T06:41:14Z

Thanks for the review @shiftkey. I will look into the issues tonight after work.

Conflicts: Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj

kristianhald · 2015-05-09T11:13:47Z

@shiftkey, review changes have been made, including a merge handling the conflicts.
I hope everything is well else you know how to get a hold of me 📧

shiftkey · 2015-05-13T03:04:09Z

Octokit.Tests.Integration/Clients/RepositoryForksClientTests.cs

+                Assert.Equal(true, forkCreated.Fork);
+            }
+
+            [IntegrationTest]


Change this attribute to [OrganizationTest] so that it only runs if you have the right environment variables set 🤘

shiftkey · 2015-05-13T03:07:48Z

Just that one test attribute and this is good to merge!

EDIT: whatever, I'm impatient. I'll fix that up on master.

Fork/Hook issue #698

shiftkey · 2015-05-13T03:09:12Z

Thanks for the amazing work @kristianhald

kristianhald · 2015-05-13T19:17:42Z

Thanks and thanks for the merge. 👌

AndyCross and others added 30 commits January 19, 2014 20:36

Implement Get Hooks on Repository Client

528d8c5

Comment missing on Client

8a51438

remove spurious comment from dev

218eeec

Include RX client

8223bad

Refactor to a RepositoryHooksClient

a13c9fa

missing test changes & correct tests which were calling Repositories.Get

8222fd9

Change method names to remove [Hooks] reference now that this is sepa…

170b0c4

…rated out - there doesn't need to be this duplicitous 'clarity' given the classes are now separate

implement members except TEST

828b776

complete test method with stub class used for serialization

7cc66aa

missing file

740fd9c

rx client fixup

5f038b8

incorrect namespace

144358b

remove references to lazy, correct inappropriate use of non-reactive …

c6f0739

…RepositoryHooksClient

refactor to remove duplication

a70fc80

address code analysis issue

3410d28

Implementation of RepositoryForks api

0c806d7

missing files

e623632

Merge from upstream master

7b54701

add missing files into mono assembly

433f182

fixup csproject files that are not part of the main .sln - xamarin st…

c597449

…udio mono things

Adding clients and models to Portable project

581cc11

Oops, missing <summary> tag

568220d

Cleaned up interfaces and implementetation so convention tests pass

7b9b06f

Cleaned up using statements

0cd5283

Updated the response classes so that they pass the convention tests a…

f559690

…nd code analysis.

The integration tests for the 'Forks' github api matching the current…

c41c4b6

… implementation of the forks api.

Removed unused reference.

36afbe5

Added 'sort' parameter to the list forks api implementation.

5f4796a

kristianhald added 6 commits April 20, 2015 21:58

Onelined the client properties.

37f4d4d

Removed unnecessary attributes.

b8b2597

Implemented data less 'Post' method and used in ping and test hook ca…

261e47f

…lls.

Changed 'dynamic' to Dictionary<string, string> for configuration of …

87c7eed

…hooks.

Manually removed link to old files that have been deleted.

ff52f19

Merge remote-tracking branch 'origin/master' into pr-495ext

336c448

Conflicts: Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj

shiftkey reviewed May 8, 2015
View reviewed changes

shiftkey added the feedback pending label May 8, 2015

shiftkey mentioned this pull request May 8, 2015

[WIP] for issue #655 Delete repository #680

Closed

kristianhald added 2 commits May 9, 2015 12:43

Review comments from 2015-05-08

bd57b08

Merge remote-tracking branch 'upstream/master' into pr-495ext

3d56bbf

Conflicts: Octokit/Octokit-MonoAndroid.csproj Octokit/Octokit-Monotouch.csproj

shiftkey mentioned this pull request May 11, 2015

Forking a repository to current user #698

Closed

shiftkey removed the feedback given label May 11, 2015

shiftkey reviewed May 13, 2015
View reviewed changes

shiftkey added a commit that referenced this pull request May 13, 2015

Merge pull request #776 from TeamBinary/pr-495ext

e4d25a3

Fork/Hook issue #698

shiftkey merged commit e4d25a3 into octokit:master May 13, 2015

shiftkey mentioned this pull request May 13, 2015

Implement Repository Forks API #336

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork/Hook issue #698 #776

Fork/Hook issue #698 #776

kristianhald commented Mar 23, 2015

kristianhald commented Apr 20, 2015

kristianhald commented Apr 28, 2015

kbilsted commented May 4, 2015

shiftkey commented May 4, 2015

kbilsted commented May 4, 2015

shiftkey May 8, 2015

kristianhald May 9, 2015

shiftkey commented May 8, 2015

shiftkey May 8, 2015

kristianhald May 9, 2015

kristianhald commented May 8, 2015

kristianhald commented May 9, 2015

shiftkey May 13, 2015

shiftkey commented May 13, 2015

shiftkey commented May 13, 2015

kristianhald commented May 13, 2015

Fork/Hook issue #698 #776

Fork/Hook issue #698 #776

Conversation

kristianhald commented Mar 23, 2015

kristianhald commented Apr 20, 2015

kristianhald commented Apr 28, 2015

kbilsted commented May 4, 2015

shiftkey commented May 4, 2015

kbilsted commented May 4, 2015

shiftkey May 8, 2015

Choose a reason for hiding this comment

kristianhald May 9, 2015

Choose a reason for hiding this comment

shiftkey commented May 8, 2015

shiftkey May 8, 2015

Choose a reason for hiding this comment

kristianhald May 9, 2015

Choose a reason for hiding this comment

kristianhald commented May 8, 2015

kristianhald commented May 9, 2015

shiftkey May 13, 2015

Choose a reason for hiding this comment

shiftkey commented May 13, 2015

shiftkey commented May 13, 2015

kristianhald commented May 13, 2015