Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

prayankmathur · 2016-03-24T15:30:58Z

Added the methods as well as tests (reactive methods too).

shiftkey · 2016-03-24T17:51:56Z

Octokit/Clients/RepositoryPagesClient.cs

+            Ensure.ArgumentNotNull(options, "options");
+
+            var endpoint = ApiUrls.RepositoryPageBuilds(owner, repositoryName);
+            return ApiConnection.GetAll<PagesBuild>(endpoint, null, AcceptHeaders.StableVersion, options);


We should be able to get away with just using the GetAll<T> overload that takes endpoint and options, right?

Yeah, my bad 😁
Have made the changes

…s well as Reactive methods and added tests

prayankmathur · 2016-03-24T18:19:16Z

@shiftkey
Made the changes. Take a look.

shiftkey · 2016-03-24T19:05:21Z

@prayankmathur looks like some tests need to be updated

prayankmathur · 2016-03-24T19:18:07Z

@shiftkey
Yeah, I was doing that.
Also, shouldn't there be a change here too, like the one that you suggested.

shiftkey · 2016-03-24T23:30:09Z

@prayankmathur you're right - I'll correct that and open a separate PR for it...

shiftkey · 2016-03-29T19:24:26Z

Octokit.Tests/Clients/RepositoryPagesClientTests.cs

@@ -31,7 +31,7 @@ public void RequestsCorrectUrl()
            }
        }

-        public class TheGetBuildsMethod
+        public class TheGetAllBuildsMethod


💄 this should be TheGetAllMethod

shiftkey · 2016-03-29T19:32:32Z

@prayankmathur this is looking good.

We don't seem to have any integration tests for the RepositoryPagesClient, but that seems to be covered by the ObservableRepositoryPagesClientTests tests. Would you mind adding that in now, at least just for the new GetAll methods we're adding here?

prayankmathur · 2016-03-30T09:53:18Z

@shiftkey
I have added the integration tests as you said. Please take a look.

ryangribble · 2016-03-30T10:35:13Z

Octokit/Clients/IRepositoryPagesClient.cs

+        /// <param name="owner">The owner of the repository</param>
+        /// <param name="repositoryName">The name of the repository</param>
+        /// <param name="options">Options for changing the behaviour of the API</param>
+        ///  <remarks>


extraneous whitespace here? 2 spaces in ///..<remarks> instead of 1.
Seems to be the case on a few of the xmldoc sections in multiple files in this PR

Also should the description of <param name="options"> be the same between observable and regular client concrete and interfaces? Currently across the 4 places, there are 3 different wordings

Options for changing the behaviour of the API

Options to change the behaviour of the API

Options to change the API response

prayankmathur · 2016-03-30T10:47:20Z

@ryangribble
I changed the description to

Options to change the API response

And i guess it should be same.

ryangribble · 2016-03-30T10:49:03Z

Octokit.Reactive/Clients/IObservableRepositoryPagesClient.cs

+        /// <param name="owner">The owner of the repository</param>
+        /// <param name="repositoryName">The name of the repository</param>
+        /// <param name="options">Options to change the API response</param>
+        ///  <remarks>


prayankmathur · 2016-03-30T10:55:21Z

@ryangribble
I have removed the whitespaces. Actually I copy pasted the description from the function above and therefore the whitespace crept in everywhere.

Please take a look.

ryangribble · 2016-03-30T12:22:20Z

Looks good now. Yeah I realise often these little inconsistencies are already in the files, but whenever we are touching files I like to clean whatever we can up as we go 👍

prayankmathur · 2016-03-30T13:38:06Z

@ryangribble
@shiftkey
Why are there no integration tests for the methods in the RespositoryPagesClient ?
I have added the tests for the GetAll
what about the remaining methods.

devkhan · 2016-03-30T14:13:41Z

@prayankmathur There is some code at places which is not covered by tests. I have encountered this situation in the TeamsClient also, so I think we should add tests wherever they are missing.

shiftkey · 2016-04-04T20:31:08Z

@prayankmathur

Why are there no integration tests for the methods in the RespositoryPagesClient?

Probably something that was missed at the time. Given this is a read-only interface it should be easy to use the Octokit.NET repository (or something similar) and craft some simple tests to get our coverage up...

prayankmathur · 2016-04-06T05:16:41Z

@shiftkey
So should i do the honors and include them too ? 😀

ryangribble · 2016-04-06T13:08:11Z

You know the answer's gonna be yes 😀 We always like moar tests

shiftkey · 2016-04-06T16:10:51Z

So should i do the honors and include them too ?

prayankmathur · 2016-04-09T14:19:08Z

@shiftkey
@ryangribble
I have added some tests for the functions.
Please take a look.
Also is there a way to check GetLatest returns the latest build, because i couldn't find a way.

ryangribble · 2016-04-09T22:28:26Z

Octokit.Tests/Clients/RepositoryPagesClientTests.cs

-                await Assert.ThrowsAsync<ArgumentNullException>(() => client.Get("owner", null));
+                await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAll(null, "name", new ApiOptions()));
+                await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAll("owner", null, new ApiOptions()));
+                await Assert.ThrowsAsync<ArgumentNullException>(() => client.GetAll("owner", "name", null));
            }


Can you also add a EnsuresNonEmptyArguments() test function that passes "" into the variuos parameters and ensures exceptions are thrown?

ryangribble · 2016-04-09T22:33:02Z

Ive made a couple of minor comments. Also it seems like there are no unit tests for Observable client.

Also is there a way to check GetLatest returns the latest build, because i couldn't find a way.

Im not too familiar with the Pages side of things, I'll have a look but it could be as you say, no way to know if it's truly the latest or not. As long as you are calling the right endpoint etc (as verified by unit tests) you have to trust the github api will return latest when we ask for it 😀

shiftkey · 2016-04-10T18:45:49Z

@ryangribble @prayankmathur looks like there's some limitations with accessing the Pages API based on the docs:

Information about the site and the builds can only be accessed by authenticated owners, even though the websites are public.

This is going to cause headaches, and perhaps we need to do this setup:

create the repository
publish something to gh-pages
check build staus
teardown the repository

This will likely mean most of the tests here are excessive...

prayankmathur · 2016-04-10T20:28:12Z

@shiftkey
So i should remove those excessive tests and then maybe open up a separate issue to create this setup ?

shiftkey · 2016-04-10T20:41:13Z

@prayankmathur that sounds great! Leave one test with an "[IntegrationTest(Skip= "These tests require repository admin rights - see https://github.com/octokit/octokit.net/issues/XXX for discussion")] so we can link to the related discussion...

prayankmathur · 2016-04-14T20:03:18Z

@shiftkey
How do I know that appveyor is running/skipping the test that i mentioned to skip ?

shiftkey · 2016-04-14T20:38:29Z

@prayankmathur it's defined in AppVeyor's config file - https://github.com/octokit/octokit.net/blob/master/appveyor.yml#L3-L7

shiftkey · 2016-05-18T09:59:23Z

I'm not sure what the current state of this PR is, so I'm moving this to #1304 so I can merge this new code in (with the skipped test).

shiftkey reviewed Mar 24, 2016
View reviewed changes

prayankmathur added 2 commits March 24, 2016 23:43

Added ApiOptions overload to method GetAll in RepositoryPagesClient a…

9c0aeca

…s well as Reactive methods and added tests

Minor fixes

1255be4

prayankmathur force-pushed the 1178 branch from 08a12c2 to 1255be4 Compare March 24, 2016 18:18

prayankmathur added 2 commits March 25, 2016 00:12

Some more fixes

2682e1e

Minor fixes

99e6d7e

Fixes

948fcdb

shiftkey mentioned this pull request Mar 25, 2016

ObservableReleasesClient.GetAll should use overloads properly #1218

Closed

1 task

shiftkey reviewed Mar 29, 2016
View reviewed changes

Added integration tests for the GetAll method

eb73e97

ryangribble reviewed Mar 30, 2016
View reviewed changes

Some changes in description

ee7dcbd

ryangribble reviewed Mar 30, 2016
View reviewed changes

Removed extraneous whitespace

2a5da0f

prayankmathur added 4 commits April 7, 2016 12:25

WIP adding integration tests

e5545c7

Resolved conflicts

c1b6f24

Modified tests

7f83e74

Fixes

8ca8227

ryangribble reviewed Apr 9, 2016
View reviewed changes

prayankmathur added 3 commits April 10, 2016 17:02

Added unit tests for Reactive functions

2f1943a

Tidying

96818f4

Fixed unit tests

9b38fff

prayankmathur added 2 commits April 11, 2016 09:49

Syncing the fork

a9fcefa

Resolved conflicts

33d237a

prayankmathur mentioned this pull request Apr 11, 2016

improve documentation for interacting with Pages API #1263

Closed

Admin rights tests stripped down

8313864

shiftkey mentioned this pull request May 18, 2016

Added ApiOption overloads to methods on I(Observable)RepositoryPagesClient #1304

Merged

shiftkey closed this May 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

prayankmathur commented Mar 24, 2016

shiftkey Mar 24, 2016

prayankmathur Mar 24, 2016

prayankmathur commented Mar 24, 2016

shiftkey commented Mar 24, 2016

prayankmathur commented Mar 24, 2016

shiftkey commented Mar 24, 2016

shiftkey Mar 29, 2016

shiftkey commented Mar 29, 2016

prayankmathur commented Mar 30, 2016

ryangribble Mar 30, 2016

prayankmathur commented Mar 30, 2016

ryangribble Mar 30, 2016

prayankmathur commented Mar 30, 2016

ryangribble commented Mar 30, 2016

prayankmathur commented Mar 30, 2016

devkhan commented Mar 30, 2016

shiftkey commented Apr 4, 2016

prayankmathur commented Apr 6, 2016

ryangribble commented Apr 6, 2016

shiftkey commented Apr 6, 2016

prayankmathur commented Apr 9, 2016

ryangribble Apr 9, 2016

ryangribble commented Apr 9, 2016

shiftkey commented Apr 10, 2016

prayankmathur commented Apr 10, 2016

shiftkey commented Apr 10, 2016

prayankmathur commented Apr 14, 2016

shiftkey commented Apr 14, 2016

shiftkey commented May 18, 2016

Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

Conversation

prayankmathur commented Mar 24, 2016

shiftkey Mar 24, 2016

Choose a reason for hiding this comment

prayankmathur Mar 24, 2016

Choose a reason for hiding this comment

prayankmathur commented Mar 24, 2016

shiftkey commented Mar 24, 2016

prayankmathur commented Mar 24, 2016

shiftkey commented Mar 24, 2016

shiftkey Mar 29, 2016

Choose a reason for hiding this comment

shiftkey commented Mar 29, 2016

prayankmathur commented Mar 30, 2016

ryangribble Mar 30, 2016

Choose a reason for hiding this comment

prayankmathur commented Mar 30, 2016

ryangribble Mar 30, 2016

Choose a reason for hiding this comment

prayankmathur commented Mar 30, 2016

ryangribble commented Mar 30, 2016

prayankmathur commented Mar 30, 2016

devkhan commented Mar 30, 2016

shiftkey commented Apr 4, 2016

prayankmathur commented Apr 6, 2016

ryangribble commented Apr 6, 2016

shiftkey commented Apr 6, 2016

prayankmathur commented Apr 9, 2016

ryangribble Apr 9, 2016

Choose a reason for hiding this comment

ryangribble commented Apr 9, 2016

shiftkey commented Apr 10, 2016

prayankmathur commented Apr 10, 2016

shiftkey commented Apr 10, 2016

prayankmathur commented Apr 14, 2016

shiftkey commented Apr 14, 2016

shiftkey commented May 18, 2016