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

Added ApiOption overloads to methods on IRepositoryPagesClient and IObservableRepositoryPagesClient #1213

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Octokit.Reactive/Clients/IObservableRepositoryPagesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public interface IObservableRepositoryPagesClient
/// <returns></returns>
[SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get")]
IObservable<Page> Get(string owner, string repositoryName);

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
Expand All @@ -26,6 +27,18 @@ public interface IObservableRepositoryPagesClient
/// </remarks>
/// <returns></returns>
IObservable<PagesBuild> GetAll(string owner, string repositoryName);

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
/// <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 response of the API</param>
/// <remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

/// See the <a href="https://developer.github.com/v3/repos/pages/#list-pages-builds">API documentation</a> for more information.
/// </remarks>
/// <returns></returns>
IObservable<PagesBuild> GetAll(string owner, string repositoryName, ApiOptions options);
/// <summary>
/// Gets the build metadata for the last build for a given repository
/// </summary>
Expand Down
21 changes: 20 additions & 1 deletion Octokit.Reactive/Clients/ObservableRepositoryPagesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,26 @@ public IObservable<PagesBuild> GetAll(string owner, string repositoryName)
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(repositoryName, "repositoryName");

return _connection.GetAndFlattenAllPages<PagesBuild>(ApiUrls.RepositoryPageBuilds(owner, repositoryName));
return GetAll(owner, repositoryName, ApiOptions.None);
}

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
/// <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 behaviour of the API</param>
/// <remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace

/// See the <a href="https://developer.github.com/v3/repos/pages/#list-pages-builds">API documentation</a> for more information.
/// </remarks>
/// <returns></returns>
public IObservable<PagesBuild> GetAll(string owner, string repositoryName, ApiOptions options)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(repositoryName, "repositoryName");
Ensure.ArgumentNotNull(options, "options");

return _connection.GetAndFlattenAllPages<PagesBuild>(ApiUrls.RepositoryPageBuilds(owner, repositoryName), options);
}

/// <summary>
Expand Down
1 change: 1 addition & 0 deletions Octokit.Tests.Integration/Octokit.Tests.Integration.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@
<Compile Include="Properties\AssemblyInfo.cs" />
<Compile Include="Helper.cs" />
<Compile Include="Clients\UsersClientTests.cs" />
<Compile Include="Reactive\ObservableRepositoryPagesClientTests.cs" />
<Compile Include="Reactive\ObservableRespositoryDeployKeysClientTests.cs" />
<Compile Include="Reactive\ObservableUserAdministrationClientTests.cs" />
<Compile Include="Reactive\ObservableUserEmailsClientTests.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
using System.Reactive.Linq;
using System.Threading.Tasks;
using Octokit.Reactive;
using Xunit;

namespace Octokit.Tests.Integration.Reactive
{
public class ObservableRepositoryPagesClientTests
{
public class TheGetAllMethod
{
readonly ObservableRepositoryPagesClient _repositoryPagesClient;
const string owner = "octokit";
const string name = "octokit.net";

public TheGetAllMethod()
{
var github = Helper.GetAuthenticatedClient();

_repositoryPagesClient = new ObservableRepositoryPagesClient(github);
}

[IntegrationTest]
public async Task ReturnsRepositoryPages()
{
var pages = await _repositoryPagesClient.GetAll(owner, name).ToList();

Assert.NotEmpty(pages);
}

[IntegrationTest]
public async Task ReturnsCorrectCountOfPagesWithoutStart()
{
var options = new ApiOptions
{
PageSize = 5,
PageCount = 1
};

var pages = await _repositoryPagesClient.GetAll(owner, name, options).ToList();
Assert.Equal(5, pages.Count);
}

[IntegrationTest]
public async Task ReturnCorrectCountOfPagesWithStart()
{
var options = new ApiOptions
{
PageSize = 5,
PageCount = 1,
StartPage = 2
};

var pages = await _repositoryPagesClient.GetAll(owner, name, options).ToList();
Assert.Equal(5, pages.Count);
}

[IntegrationTest]
public async Task ReturnsDistinctResultsBasedOnStartPage()
{
var startOptions = new ApiOptions
{
PageSize = 5,
PageCount = 1
};

var firstPage = await _repositoryPagesClient.GetAll(owner, name, startOptions).ToList();

var skipStartOptions = new ApiOptions
{
PageSize = 5,
PageCount = 1,
StartPage = 2
};

var secondPage = await _repositoryPagesClient.GetAll(owner, name, skipStartOptions).ToList();

Assert.NotEqual(firstPage[0].Url, secondPage[0].Url);
Assert.NotEqual(firstPage[1].Url, secondPage[1].Url);
Assert.NotEqual(firstPage[2].Url, secondPage[2].Url);
Assert.NotEqual(firstPage[3].Url, secondPage[3].Url);
Assert.NotEqual(firstPage[4].Url, secondPage[4].Url);
}
}
}
}
9 changes: 5 additions & 4 deletions Octokit.Tests/Clients/RepositoryPagesClientTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public async Task EnsuresNonNullArguments()
}
}

public class TheGetBuildsMethod
public class TheGetAllBuildsMethod
Copy link
Member

Choose a reason for hiding this comment

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

💄 this should be TheGetAllMethod

{
[Fact]
public void RequestsCorrectUrl()
Expand All @@ -41,7 +41,7 @@ public void RequestsCorrectUrl()

client.GetAll("fake", "repo");

connection.Received().GetAll<PagesBuild>(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/pages/builds"));
connection.Received().GetAll<PagesBuild>(Arg.Is<Uri>(u => u.ToString() == "repos/fake/repo/pages/builds"), Args.ApiOptions);
}

[Fact]
Expand All @@ -50,8 +50,9 @@ public async Task EnsuresNonNullArguments()
var connection = Substitute.For<IApiConnection>();
var client = new RepositoryPagesClient(connection);

await Assert.ThrowsAsync<ArgumentNullException>(() => client.Get(null, "name"));
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));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

Expand Down
14 changes: 14 additions & 0 deletions Octokit/Clients/IRepositoryPagesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public interface IRepositoryPagesClient
/// <returns></returns>
[SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get")]
Task<Page> Get(string owner, string repositoryName);

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
Expand All @@ -33,6 +34,19 @@ public interface IRepositoryPagesClient
/// </remarks>
/// <returns></returns>
Task<IReadOnlyList<PagesBuild>> GetAll(string owner, string repositoryName);

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
/// <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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

/// See the <a href="https://developer.github.com/v3/repos/pages/#list-pages-builds">API documentation</a> for more information.
/// </remarks>
/// <returns></returns>
Task<IReadOnlyList<PagesBuild>> GetAll(string owner, string repositoryName, ApiOptions options);

/// <summary>
/// Gets the build metadata for the last build for a given repository
/// </summary>
Expand Down
21 changes: 20 additions & 1 deletion Octokit/Clients/RepositoryPagesClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,28 @@ public Task<IReadOnlyList<PagesBuild>> GetAll(string owner, string repositoryNam
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(repositoryName, "repositoryName");

return ApiConnection.GetAll<PagesBuild>(ApiUrls.RepositoryPageBuilds(owner, repositoryName));
return GetAll(owner, repositoryName, ApiOptions.None);
}

/// <summary>
/// Gets all build metadata for a given repository
/// </summary>
/// <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>
/// See the <a href="https://developer.github.com/v3/repos/pages/#list-pages-builds">API documentation</a> for more information.
/// </remarks>
/// <returns></returns>
public Task<IReadOnlyList<PagesBuild>> GetAll(string owner, string repositoryName, ApiOptions options)
{
Ensure.ArgumentNotNullOrEmptyString(owner, "owner");
Ensure.ArgumentNotNullOrEmptyString(repositoryName, "repositoryName");
Ensure.ArgumentNotNull(options, "options");

var endpoint = ApiUrls.RepositoryPageBuilds(owner, repositoryName);
return ApiConnection.GetAll<PagesBuild>(endpoint, options);
}
/// <summary>
/// Gets the build metadata for the last build for a given repository
/// </summary>
Expand Down