From 2b9f83b876fb475385f36042c7fdfbbdea8522d9 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 19 Mar 2015 16:07:33 +0930 Subject: [PATCH 01/16] add ApiOptions to support tailoring the response details --- Octokit/Models/Request/ApiOptions.cs | 62 ++++++++++++++++++++++++++++ Octokit/Octokit-Mono.csproj | 1 + Octokit/Octokit-MonoAndroid.csproj | 1 + Octokit/Octokit-Monotouch.csproj | 1 + Octokit/Octokit-Portable.csproj | 1 + Octokit/Octokit-netcore45.csproj | 1 + Octokit/Octokit.csproj | 1 + 7 files changed, 68 insertions(+) create mode 100644 Octokit/Models/Request/ApiOptions.cs diff --git a/Octokit/Models/Request/ApiOptions.cs b/Octokit/Models/Request/ApiOptions.cs new file mode 100644 index 00000000..ef149633 --- /dev/null +++ b/Octokit/Models/Request/ApiOptions.cs @@ -0,0 +1,62 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; + +namespace Octokit +{ + [DebuggerDisplay("{DebuggerDisplay,nq}")] + public class ApiOptions + { + public static ApiOptions None + { + get { return new ApiOptions(); } + } + + /// + /// Specify the start page for pagination actions + /// + /// + /// Page numbering is 1-based on the server + /// + public int? StartPage { get; set; } + + /// + /// Specify the number of pages to return + /// + public int? PageCount { get; set; } + + /// + /// Specify the number of results to return for each page + /// + /// + /// Results returned may be less than this total if you reach the final page of results + /// + public int? PageSize { get; set; } + + internal string DebuggerDisplay + { + get + { + var values = new List(); + + if (StartPage.HasValue) + { + values.Add("StartPage: " + StartPage.Value); + } + + if (PageCount.HasValue) + { + values.Add("PageCount: " + PageCount.Value); + } + + if (PageSize.HasValue) + { + values.Add("PageSize: " + PageSize.Value); + } + + return String.Join(", ", values); + } + } + } + +} diff --git a/Octokit/Octokit-Mono.csproj b/Octokit/Octokit-Mono.csproj index cf4adab8..c518b97d 100644 --- a/Octokit/Octokit-Mono.csproj +++ b/Octokit/Octokit-Mono.csproj @@ -447,6 +447,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-MonoAndroid.csproj b/Octokit/Octokit-MonoAndroid.csproj index ca3641e9..904a34c0 100644 --- a/Octokit/Octokit-MonoAndroid.csproj +++ b/Octokit/Octokit-MonoAndroid.csproj @@ -455,6 +455,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-Monotouch.csproj b/Octokit/Octokit-Monotouch.csproj index 9b911c87..9816e046 100644 --- a/Octokit/Octokit-Monotouch.csproj +++ b/Octokit/Octokit-Monotouch.csproj @@ -451,6 +451,7 @@ + diff --git a/Octokit/Octokit-Portable.csproj b/Octokit/Octokit-Portable.csproj index 54301735..f2faa095 100644 --- a/Octokit/Octokit-Portable.csproj +++ b/Octokit/Octokit-Portable.csproj @@ -444,6 +444,7 @@ + diff --git a/Octokit/Octokit-netcore45.csproj b/Octokit/Octokit-netcore45.csproj index 47276432..2a4d4aa8 100644 --- a/Octokit/Octokit-netcore45.csproj +++ b/Octokit/Octokit-netcore45.csproj @@ -451,6 +451,7 @@ + diff --git a/Octokit/Octokit.csproj b/Octokit/Octokit.csproj index d293cab7..22536261 100644 --- a/Octokit/Octokit.csproj +++ b/Octokit/Octokit.csproj @@ -110,6 +110,7 @@ + From f9f72106be7f5a4b6842b20b031b824425e5704e Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 19 Mar 2015 17:27:39 +0930 Subject: [PATCH 02/16] implemented a test to verify that overloads are implemented for GetAll usages --- .../Exception/ApiOptionsMissingException.cs | 32 ++++++++ .../Octokit.Tests.Conventions.csproj | 2 + Octokit.Tests.Conventions/PaginationTests.cs | 75 +++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100644 Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs create mode 100644 Octokit.Tests.Conventions/PaginationTests.cs diff --git a/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs b/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs new file mode 100644 index 00000000..f913ab94 --- /dev/null +++ b/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs @@ -0,0 +1,32 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; + +namespace Octokit.Tests.Conventions +{ + public class ApiOptionsMissingException : Exception + { + public ApiOptionsMissingException(Type type, IEnumerable methods) + : base(CreateMessage(type, methods)) { } + + static string CreateMessage(Type type, IEnumerable methods) + { + var methodsFormatted = String.Join("\r\n", methods.Select(FormatMethod)); + return "Methods found on type {0} require an overload which accepts an parameter of type ApiOptions:\r\n{1}" + .FormatWithNewLine( + type.Name, + methodsFormatted); + } + + static string FormatMethod(MethodInfo m) + { + var parametersFormatteds = m.GetParameters() + .Select(p => string.Format("{0} {1}", p.ParameterType.Name, p.Name)); + + var parameterList = string.Join(", ", parametersFormatteds); + + return string.Format(" - {0}({1})", m.Name, parameterList); + } + } +} diff --git a/Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj b/Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj index fe11b292..c6012156 100644 --- a/Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj +++ b/Octokit.Tests.Conventions/Octokit.Tests.Conventions.csproj @@ -68,6 +68,7 @@ + @@ -76,6 +77,7 @@ + diff --git a/Octokit.Tests.Conventions/PaginationTests.cs b/Octokit.Tests.Conventions/PaginationTests.cs new file mode 100644 index 00000000..135d7bc2 --- /dev/null +++ b/Octokit.Tests.Conventions/PaginationTests.cs @@ -0,0 +1,75 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using Xunit; + +namespace Octokit.Tests.Conventions +{ + public class PaginationTests + { + [Theory(Skip = "Disable this to run it and find all the places where things break")] + [MemberData("GetClientInterfaces")] + public void CheckObservableClients(Type clientInterface) + { + var methodsOrdered = clientInterface.GetMethodsOrdered(); + + var methodsWhichCanPaginate = methodsOrdered + .Where(x => x.Name.StartsWith("GetAll")); + + var invalidMethods = methodsWhichCanPaginate + .Where(method => MethodHasAppropriateOverload(method, methodsOrdered) == null) + .ToList(); + + if (invalidMethods.Any()) + { + throw new ApiOptionsMissingException(clientInterface, invalidMethods); + } + } + + static MethodInfo MethodHasAppropriateOverload(MethodInfo method, MethodInfo[] methodsOrdered) + { + var parameters = method.GetParametersOrdered(); + var name = method.Name; + return methodsOrdered + .Where(x => x.Name == name) + .FirstOrDefault(x => MethodHasOverloadForApiOptions(x, parameters)); + } + + static bool MethodHasOverloadForApiOptions(MethodInfo methodInfo, ParameterInfo[] expected) + { + var actual = methodInfo.GetParameters(); + + if (actual.Length != expected.Length + 1) + { + return false; + } + + for (var i = 0; i < expected.Length; i++) + { + var a = actual.ElementAt(i); + var e = expected.ElementAt(i); + + if (a.Name != e.Name) + { + return false; + } + + if (a.ParameterType != e.ParameterType) + { + return false; + } + } + + var lastParameter = actual.Last(); + + return lastParameter.Name == "options" + && lastParameter.ParameterType == typeof(ApiOptions); + } + + public static IEnumerable GetClientInterfaces() + { + return typeof(IEventsClient).Assembly.ExportedTypes.Where(TypeExtensions.IsClientInterface).Select(type => new[] { type }); + } + } +} From 740b70f3c72d586762663ecc4f9760a34282db9d Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 15:00:08 +1100 Subject: [PATCH 03/16] moved test to new home --- Octokit.Tests.Conventions/ModelTests.cs | 21 ---------------- Octokit.Tests.Conventions/PaginationTests.cs | 25 +++++++++++++++++++- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/Octokit.Tests.Conventions/ModelTests.cs b/Octokit.Tests.Conventions/ModelTests.cs index f126e8cd..0cfcb3c8 100644 --- a/Octokit.Tests.Conventions/ModelTests.cs +++ b/Octokit.Tests.Conventions/ModelTests.cs @@ -95,27 +95,6 @@ namespace Octokit.Tests.Conventions } } - //TODO: This should (probably) be moved to the PaginationTests class that is being introduced in PR #760 - [Theory] - [MemberData("GetClientInterfaces")] - public void CheckPaginationGetAllMethodNames(Type clientInterface) - { - var methodsOrdered = clientInterface.GetMethodsOrdered(); - - var methodsThatCanPaginate = methodsOrdered - .Where(x => x.ReturnType.GetTypeInfo().TypeCategory == TypeCategory.ReadOnlyList) - .Where(x => x.Name.StartsWith("Get")); - - var invalidMethods = methodsThatCanPaginate - .Where(x => !x.Name.StartsWith("GetAll")) - .ToList(); - - if (invalidMethods.Any()) - { - throw new PaginationGetAllMethodNameMismatchException(clientInterface, invalidMethods); - } - } - public static IEnumerable GetClientInterfaces() { return typeof(IEventsClient) diff --git a/Octokit.Tests.Conventions/PaginationTests.cs b/Octokit.Tests.Conventions/PaginationTests.cs index 135d7bc2..fcd76b63 100644 --- a/Octokit.Tests.Conventions/PaginationTests.cs +++ b/Octokit.Tests.Conventions/PaginationTests.cs @@ -27,6 +27,26 @@ namespace Octokit.Tests.Conventions } } + [Theory] + [MemberData("GetClientInterfaces")] + public void CheckPaginationGetAllMethodNames(Type clientInterface) + { + var methodsOrdered = clientInterface.GetMethodsOrdered(); + + var methodsThatCanPaginate = methodsOrdered + .Where(x => x.ReturnType.GetTypeInfo().TypeCategory == TypeCategory.ReadOnlyList) + .Where(x => x.Name.StartsWith("Get")); + + var invalidMethods = methodsThatCanPaginate + .Where(x => !x.Name.StartsWith("GetAll")) + .ToList(); + + if (invalidMethods.Any()) + { + throw new PaginationGetAllMethodNameMismatchException(clientInterface, invalidMethods); + } + } + static MethodInfo MethodHasAppropriateOverload(MethodInfo method, MethodInfo[] methodsOrdered) { var parameters = method.GetParametersOrdered(); @@ -69,7 +89,10 @@ namespace Octokit.Tests.Conventions public static IEnumerable GetClientInterfaces() { - return typeof(IEventsClient).Assembly.ExportedTypes.Where(TypeExtensions.IsClientInterface).Select(type => new[] { type }); + return typeof(IEventsClient).Assembly.ExportedTypes + .Where(TypeExtensions.IsClientInterface) + .Where(type => type != typeof(IStatisticsClient)) + .Select(type => new[] { type }); } } } From c098ecda60e4df23a59f8e05846508d326aae012 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 15:29:02 +1100 Subject: [PATCH 04/16] added overloads to IApiConnection which support passing in ApiOptions --- .../Clients/AuthorizationsClientTests.cs | 2 +- Octokit.Tests/Http/ApiConnectionTests.cs | 4 +- Octokit/Clients/AuthorizationsClient.cs | 2 +- Octokit/Helpers/ApiExtensions.cs | 2 +- Octokit/Http/ApiConnection.cs | 110 +++++++++++++++++- Octokit/Http/IApiConnection.cs | 33 ++++++ 6 files changed, 146 insertions(+), 7 deletions(-) diff --git a/Octokit.Tests/Clients/AuthorizationsClientTests.cs b/Octokit.Tests/Clients/AuthorizationsClientTests.cs index af18967e..1a05b4f4 100644 --- a/Octokit.Tests/Clients/AuthorizationsClientTests.cs +++ b/Octokit.Tests/Clients/AuthorizationsClientTests.cs @@ -35,7 +35,7 @@ namespace Octokit.Tests.Clients client.Received().GetAll( Arg.Is(u => u.ToString() == "authorizations"), - null); + Arg.Is>(d => d == null)); } } diff --git a/Octokit.Tests/Http/ApiConnectionTests.cs b/Octokit.Tests/Http/ApiConnectionTests.cs index f0dd7b6d..5f3b7d22 100644 --- a/Octokit.Tests/Http/ApiConnectionTests.cs +++ b/Octokit.Tests/Http/ApiConnectionTests.cs @@ -90,13 +90,13 @@ namespace Octokit.Tests.Http new Response(), new List { new object(), new object() }); var connection = Substitute.For(); - connection.Get>(Args.Uri, null, null).Returns(Task.FromResult(response)); + connection.Get>(Args.Uri, Args.EmptyDictionary, null).Returns(Task.FromResult(response)); var apiConnection = new ApiConnection(connection); var data = await apiConnection.GetAll(getAllUri); Assert.Equal(2, data.Count); - connection.Received().Get>(getAllUri, null, null); + connection.Received().Get>(getAllUri, Args.EmptyDictionary, null); } [Fact] diff --git a/Octokit/Clients/AuthorizationsClient.cs b/Octokit/Clients/AuthorizationsClient.cs index 440918a5..e7fd862e 100644 --- a/Octokit/Clients/AuthorizationsClient.cs +++ b/Octokit/Clients/AuthorizationsClient.cs @@ -36,7 +36,7 @@ namespace Octokit /// A list of s. public Task> GetAll() { - return ApiConnection.GetAll(ApiUrls.Authorizations(), null); + return ApiConnection.GetAll(ApiUrls.Authorizations(), (IDictionary)null); } /// diff --git a/Octokit/Helpers/ApiExtensions.cs b/Octokit/Helpers/ApiExtensions.cs index 5a3c9a15..81b1ab8e 100644 --- a/Octokit/Helpers/ApiExtensions.cs +++ b/Octokit/Helpers/ApiExtensions.cs @@ -26,7 +26,7 @@ namespace Octokit Ensure.ArgumentNotNull(connection, "connection"); Ensure.ArgumentNotNull(uri, "uri"); - return connection.GetAll(uri, null); + return connection.GetAll(uri, ApiOptions.None); } /// diff --git a/Octokit/Http/ApiConnection.cs b/Octokit/Http/ApiConnection.cs index 5821e327..179d898d 100644 --- a/Octokit/Http/ApiConnection.cs +++ b/Octokit/Http/ApiConnection.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Globalization; +using System.Linq; using System.Net; using System.Threading; using System.Threading.Tasks; @@ -115,7 +117,20 @@ namespace Octokit /// Thrown when an API error occurs. public Task> GetAll(Uri uri) { - return GetAll(uri, null, null); + return GetAll(uri, ApiOptions.None); + } + + /// + /// Gets all API resources in the list at the specified URI. + /// + /// Type of the API resource in the list. + /// URI of the API resource to get + /// Options for changing the API response + /// of the The API resources in the list. + /// Thrown when an API error occurs. + public Task> GetAll(Uri uri, ApiOptions options) + { + return GetAll(uri, null, null, options); } /// @@ -128,7 +143,21 @@ namespace Octokit /// Thrown when an API error occurs. public Task> GetAll(Uri uri, IDictionary parameters) { - return GetAll(uri, parameters, null); + return GetAll(uri, parameters, null, ApiOptions.None); + } + + /// + /// Gets all API resources in the list at the specified URI. + /// + /// Type of the API resource in the list. + /// URI of the API resource to get + /// Parameters to add to the API request + /// Options for changing the API response + /// of the The API resources in the list. + /// Thrown when an API error occurs. + public Task> GetAll(Uri uri, IDictionary parameters, ApiOptions options) + { + return GetAll(uri, parameters, null, options); } /// @@ -148,6 +177,27 @@ namespace Octokit .ConfigureAwait(false), uri); } + public Task> GetAll(Uri uri, IDictionary parameters, string accepts, ApiOptions options) + { + Ensure.ArgumentNotNull(uri, "uri"); + Ensure.ArgumentNotNull(options, "options"); + + parameters = parameters ?? new Dictionary(); + + if (options.PageSize.HasValue) + { + parameters.Add("per_page", options.PageSize.Value.ToString(CultureInfo.InvariantCulture)); + } + + if (options.StartPage.HasValue) + { + parameters.Add("page", options.StartPage.Value.ToString(CultureInfo.InvariantCulture)); + } + + return _pagination.GetAllPages(async () => await GetPage(uri, parameters, accepts, options) + .ConfigureAwait(false), uri); + } + /// /// Creates a new API resource in the list at the specified URI. /// @@ -483,5 +533,61 @@ namespace Octokit response, nextPageUri => Connection.Get>(nextPageUri, parameters, accepts)); } + + async Task> GetPage( + Uri uri, + IDictionary parameters, + string accepts, + ApiOptions options) + { + Ensure.ArgumentNotNull(uri, "uri"); + + var connection = Connection; + + var response = await connection.Get>(uri, parameters, accepts).ConfigureAwait(false); + return new ReadOnlyPagedCollection( + response, + nextPageUri => + { + if (nextPageUri.Query.Contains("page=") && options.PageCount.HasValue) + { + var allValues = ToQueryStringDictionary(nextPageUri); + + string pageValue; + if (allValues.TryGetValue("page", out pageValue)) + { + var startPage = options.StartPage ?? 1; + var pageCount = options.PageCount.Value; + + var endPage = startPage + pageCount; + if (pageValue.Equals(endPage.ToString(), StringComparison.OrdinalIgnoreCase)) + { + return null; + } + } + } + + return connection.Get>(nextPageUri, parameters, accepts); + }); + } + + static Dictionary ToQueryStringDictionary(Uri uri) + { + return uri.Query.Split('&') + .Select(keyValue => + { + var indexOf = keyValue.IndexOf('='); + if (indexOf > 0) + { + var key = keyValue.Substring(0, indexOf); + var value = keyValue.Substring(indexOf + 1); + return new KeyValuePair(key, value); + } + + //just a plain old value, return it + return new KeyValuePair(keyValue, null); + }) + .ToDictionary(x => x.Key, x => x.Value); + } } } diff --git a/Octokit/Http/IApiConnection.cs b/Octokit/Http/IApiConnection.cs index c111707b..37239067 100644 --- a/Octokit/Http/IApiConnection.cs +++ b/Octokit/Http/IApiConnection.cs @@ -71,6 +71,16 @@ namespace Octokit /// Thrown when an API error occurs. Task> GetAll(Uri uri); + /// + /// Gets all API resources in the list at the specified URI. + /// + /// Type of the API resource in the list. + /// URI of the API resource to get + /// Options for changing the API response + /// of the The API resources in the list. + /// Thrown when an API error occurs. + Task> GetAll(Uri uri, ApiOptions options); + /// /// Gets all API resources in the list at the specified URI. /// @@ -81,6 +91,17 @@ namespace Octokit /// Thrown when an API error occurs. Task> GetAll(Uri uri, IDictionary parameters); + /// + /// Gets all API resources in the list at the specified URI. + /// + /// Type of the API resource in the list. + /// URI of the API resource to get + /// Parameters to add to the API request + /// Options for changing the API response + /// of the The API resources in the list. + /// Thrown when an API error occurs. + Task> GetAll(Uri uri, IDictionary parameters, ApiOptions options); + /// /// Gets all API resources in the list at the specified URI. /// @@ -92,6 +113,18 @@ namespace Octokit /// Thrown when an API error occurs. Task> GetAll(Uri uri, IDictionary parameters, string accepts); + /// + /// Gets all API resources in the list at the specified URI. + /// + /// Type of the API resource in the list. + /// URI of the API resource to get + /// Parameters to add to the API request + /// Accept header to use for the API request + /// Options for changing the API response + /// of the The API resources in the list. + /// Thrown when an API error occurs. + Task> GetAll(Uri uri, IDictionary parameters, string accepts, ApiOptions options); + /// /// Creates a new API resource in the list at the specified URI. /// From 03ea83148995711fc47148489e2123e65f33bd82 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 15:46:41 +1100 Subject: [PATCH 05/16] implemented IReleaseClient.GetAll with overloads to take ApiOptions --- Octokit.Tests/Clients/ReleasesClientTests.cs | 6 +++--- Octokit.Tests/Helpers/Arg.cs | 5 +++++ Octokit/Clients/IReleasesClient.cs | 13 ++++++++++++ Octokit/Clients/ReleasesClient.cs | 22 +++++++++++++++++++- 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/Octokit.Tests/Clients/ReleasesClientTests.cs b/Octokit.Tests/Clients/ReleasesClientTests.cs index b78ec427..bb0ef316 100644 --- a/Octokit.Tests/Clients/ReleasesClientTests.cs +++ b/Octokit.Tests/Clients/ReleasesClientTests.cs @@ -2,14 +2,13 @@ using System.IO; using System.Threading.Tasks; using NSubstitute; -using Octokit.Tests.Helpers; using Xunit; namespace Octokit.Tests.Clients { public class ReleasesClientTests { - public class TheGetReleasesMethod + public class TheGetAllMethod { [Fact] public void RequestsCorrectUrl() @@ -21,7 +20,8 @@ namespace Octokit.Tests.Clients client.Received().GetAll(Arg.Is(u => u.ToString() == "repos/fake/repo/releases"), null, - "application/vnd.github.v3"); + AcceptHeaders.StableVersion, + Args.ApiOptions); } [Fact] diff --git a/Octokit.Tests/Helpers/Arg.cs b/Octokit.Tests/Helpers/Arg.cs index 391255fa..5408f72d 100644 --- a/Octokit.Tests/Helpers/Arg.cs +++ b/Octokit.Tests/Helpers/Arg.cs @@ -67,5 +67,10 @@ namespace Octokit.Tests { get { return Arg.Any(); } } + + public static ApiOptions ApiOptions + { + get { return Arg.Any(); } + } } } diff --git a/Octokit/Clients/IReleasesClient.cs b/Octokit/Clients/IReleasesClient.cs index d04b0cd3..435f67ae 100644 --- a/Octokit/Clients/IReleasesClient.cs +++ b/Octokit/Clients/IReleasesClient.cs @@ -26,6 +26,19 @@ namespace Octokit /// The list of s for the specified repository. Task> GetAll(string owner, string name); + /// + /// Gets all s for the specified repository. + /// + /// + /// See the API documentation for more information. + /// + /// The repository's owner + /// The repository's name + /// Options for changing the API response + /// Thrown when a general API error occurs. + /// The list of s for the specified repository. + Task> GetAll(string owner, string name, ApiOptions options); + /// /// Gets a single for the specified repository. /// diff --git a/Octokit/Clients/ReleasesClient.cs b/Octokit/Clients/ReleasesClient.cs index fae7bc4d..7fd95fea 100644 --- a/Octokit/Clients/ReleasesClient.cs +++ b/Octokit/Clients/ReleasesClient.cs @@ -36,8 +36,28 @@ namespace Octokit Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); Ensure.ArgumentNotNullOrEmptyString(name, "repository"); + return GetAll(owner, name, ApiOptions.None); + } + + /// + /// Gets all s for the specified repository. + /// + /// + /// See the API documentation for more information. + /// + /// The repository's owner + /// The repository's name + /// Options for changing the API response + /// Thrown when a general API error occurs. + /// The list of s for the specified repository. + public Task> GetAll(string owner, string name, ApiOptions options) + { + Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); + Ensure.ArgumentNotNullOrEmptyString(name, "repository"); + Ensure.ArgumentNotNull(options, "options"); + var endpoint = ApiUrls.Releases(owner, name); - return ApiConnection.GetAll(endpoint, null, AcceptHeaders.StableVersion); + return ApiConnection.GetAll(endpoint, null, AcceptHeaders.StableVersion, options); } /// From 4da9f3e8f76bca109b74925774ab9864a58d761f Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 15:47:29 +1100 Subject: [PATCH 06/16] wrote some tests to verify the behaviour of the pagination --- .../Clients/ReleasesClientTests.cs | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) diff --git a/Octokit.Tests.Integration/Clients/ReleasesClientTests.cs b/Octokit.Tests.Integration/Clients/ReleasesClientTests.cs index 0dd4df70..34892219 100644 --- a/Octokit.Tests.Integration/Clients/ReleasesClientTests.cs +++ b/Octokit.Tests.Integration/Clients/ReleasesClientTests.cs @@ -75,6 +75,83 @@ public class ReleasesClientTests } } + public class TheGetAllMethod + { + readonly IReleasesClient _releaseClient; + const string owner = "octokit"; + const string name = "octokit.net"; + + public TheGetAllMethod() + { + var github = Helper.GetAuthenticatedClient(); + _releaseClient = github.Repository.Release; + } + + [IntegrationTest] + public async Task ReturnsReleases() + { + var releases = await _releaseClient.GetAll(owner, name); + + Assert.NotEmpty(releases); + } + + [IntegrationTest] + public async Task ReturnsCorrectCountOfReleasesWithoutStart() + { + var options = new ApiOptions + { + PageSize = 5, + PageCount = 1 + }; + + var releases = await _releaseClient.GetAll(owner, name, options); + + Assert.Equal(5, releases.Count); + } + + [IntegrationTest] + public async Task ReturnsCorrectCountOfReleasesWithStart() + { + var options = new ApiOptions + { + PageSize = 5, + PageCount = 1, + StartPage = 2 + }; + + var releases = await _releaseClient.GetAll(owner, name, options); + + Assert.Equal(5, releases.Count); + } + + [IntegrationTest] + public async Task ReturnsDistinctResultsBasedOnStartPage() + { + var startOptions = new ApiOptions + { + PageSize = 5, + PageCount = 1 + }; + + var firstPage = await _releaseClient.GetAll(owner, name, startOptions); + + var skipStartOptions = new ApiOptions + { + PageSize = 5, + PageCount = 1, + StartPage = 2 + }; + + var secondPage = await _releaseClient.GetAll(owner, name, skipStartOptions); + + Assert.NotEqual(firstPage[0].Id, secondPage[0].Id); + Assert.NotEqual(firstPage[1].Id, secondPage[1].Id); + Assert.NotEqual(firstPage[2].Id, secondPage[2].Id); + Assert.NotEqual(firstPage[3].Id, secondPage[3].Id); + Assert.NotEqual(firstPage[4].Id, secondPage[4].Id); + } + } + public class TheEditMethod : IDisposable { private readonly IGitHubClient _github; From a4765c8b630691852ab859a604448e132da90d17 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 16:09:07 +1100 Subject: [PATCH 07/16] HACK: i'm abusing _nextPageFun to help identify when to exit --- Octokit/Http/ReadOnlyPagedCollection.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Octokit/Http/ReadOnlyPagedCollection.cs b/Octokit/Http/ReadOnlyPagedCollection.cs index 9e642824..520f107b 100644 --- a/Octokit/Http/ReadOnlyPagedCollection.cs +++ b/Octokit/Http/ReadOnlyPagedCollection.cs @@ -28,7 +28,14 @@ namespace Octokit.Internal var nextPageUrl = _info.GetNextPageUrl(); if (nextPageUrl == null) return null; - var response = await _nextPageFunc(nextPageUrl).ConfigureAwait(false); + var maybeTask = _nextPageFunc(nextPageUrl); + + if (maybeTask == null) + { + return null; + } + + var response = await maybeTask.ConfigureAwait(false); return new ReadOnlyPagedCollection(response, _nextPageFunc); } } From 0ea28a94f386de546849ee388c4c1e4975875e61 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 16:26:25 +1100 Subject: [PATCH 08/16] added some basic documentation --- docs/extensibility.md | 25 +++++++++++++++++++++++++ mkdocs.yml | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 docs/extensibility.md diff --git a/docs/extensibility.md b/docs/extensibility.md new file mode 100644 index 00000000..abab1675 --- /dev/null +++ b/docs/extensibility.md @@ -0,0 +1,25 @@ +# Extensibility + +Octokit.net has been designed to be easy to get started, but there are options +available to tweak the default behaviour once you know the basics. + +## Pagination + +The GitHub API supports paging results whenever collections are returned. + +By default, Octokit.net will fetch the entire set of data. Any method prefixed with +`GetAll*` now has an overload which accepts an `ApiOptions` parameter. + +```csharp +var options = new ApiOptions(); +var repositories = await client.Repository.GetAllForCurrent(options); +``` + +`ApiOptions` has a number of properties: + + - `PageCount` - return a set number of pages + - `PageSize` - change the number of results to return per page + - `StartPage` - start results from a given page + +These parameters can be used in any sort of group. If you don't specify a +`PageSize` the default page size is 30. diff --git a/mkdocs.yml b/mkdocs.yml index 713d7cbf..a2ea05de 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -29,8 +29,9 @@ pages: - 'Exploring Pull Requests' : 'demos/exploring-pull-requests.md' - Advanced: + - 'API Options': 'extensibility.md' - 'Debugging from Source': 'debugging-source.md' - - 'OAuth Flow': 'oauth-flow.md' + - 'HttpClient': 'http-client.md' - 'HttpClient': 'http-client.md' - Contributing: From 3a9dcc33f72631c7bcb61b9d8a297c352ee3099f Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 16:48:11 +1100 Subject: [PATCH 09/16] added tests for new behaviour within pagination --- .../Models/ReadOnlyPagedCollectionTests.cs | 64 +++++++++++++++++++ mkdocs.yml | 2 +- 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs b/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs index 6fa1e4c2..6b690a63 100644 --- a/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs +++ b/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs @@ -33,6 +33,70 @@ namespace Octokit.Tests.Models Assert.NotNull(nextPage); Assert.Equal(2, nextPage.Count); } + + [Fact] + public async Task WhenNoInformationSetReturnsNull() + { + var nextPageUrl = new Uri("https://example.com/page/2"); + var nextPageResponse = Task.Factory.StartNew>>(() => + new ApiResponse>(new Response(), new List { new object(), new object() })); + + var links = new Dictionary(); + var scopes = new List(); + var httpResponse = Substitute.For(); + httpResponse.ApiInfo.Returns(new ApiInfo(links, scopes, scopes, "etag", new RateLimit(new Dictionary()))); + + var response = new ApiResponse>(httpResponse, new List()); + var connection = Substitute.For(); + + connection.Get>(nextPageUrl, null, null).Returns(nextPageResponse); + + var pagedCollection = new ReadOnlyPagedCollection( + response, + nextPageUri => connection.Get>(nextPageUrl, null, null)); + + var nextPage = await pagedCollection.GetNextPage(); + + Assert.Null(nextPage); + } + + [Fact] + public async Task WhenInlineFuncKillsPaginationReturnNull() + { + var nextPageUrl = new Uri("https://example.com/page/2"); + var nextPageResponse = Task.Factory.StartNew>>(() => + new ApiResponse>(new Response(), new List { new object(), new object() })); + + var links = new Dictionary { { "next", nextPageUrl } }; + var scopes = new List(); + var httpResponse = Substitute.For(); + httpResponse.ApiInfo.Returns(new ApiInfo(links, scopes, scopes, "etag", new RateLimit(new Dictionary()))); + + var response = new ApiResponse>(httpResponse, new List()); + var connection = Substitute.For(); + + connection.Get>(nextPageUrl, null, null).Returns(nextPageResponse); + + var pageCount = 0; + + var pagedCollection = new ReadOnlyPagedCollection( + response, + nextPageUri => + { + if (pageCount > 1) + { + return null; + } + pageCount++; + return connection.Get>(nextPageUrl, null, null); + }); + + var first = await pagedCollection.GetNextPage(); + var second = await pagedCollection.GetNextPage(); + + Assert.NotNull(first); + Assert.NotNull(second); + } } } } diff --git a/mkdocs.yml b/mkdocs.yml index a2ea05de..3ac0cf66 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -31,7 +31,7 @@ pages: - Advanced: - 'API Options': 'extensibility.md' - 'Debugging from Source': 'debugging-source.md' - - 'HttpClient': 'http-client.md' + - 'OAuth Flow': 'oauth-flow.md' - 'HttpClient': 'http-client.md' - Contributing: From aa3ce0db9210b778414e290af7f17c4a646bdeec Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 16:54:27 +1100 Subject: [PATCH 10/16] updated IObservableReleasesClient to use ApiOptions, and added some tests --- .../Clients/IObservableReleasesClient.cs | 13 +++ .../Clients/ObservableReleasesClient.cs | 20 +++++ .../Helpers/ConnectionExtensions.cs | 77 ++++++++++++++++ .../Octokit.Tests.Integration.csproj | 1 + .../Reactive/ObservableReleaseClientTests.cs | 88 +++++++++++++++++++ 5 files changed, 199 insertions(+) create mode 100644 Octokit.Tests.Integration/Reactive/ObservableReleaseClientTests.cs diff --git a/Octokit.Reactive/Clients/IObservableReleasesClient.cs b/Octokit.Reactive/Clients/IObservableReleasesClient.cs index a8bfa4cc..242e13d2 100644 --- a/Octokit.Reactive/Clients/IObservableReleasesClient.cs +++ b/Octokit.Reactive/Clients/IObservableReleasesClient.cs @@ -18,6 +18,19 @@ namespace Octokit.Reactive /// The list of s for the specified repository. IObservable GetAll(string owner, string name); + /// + /// Gets all s for the specified repository. + /// + /// + /// See the API documentation for more information. + /// + /// The repository's owner + /// The repository's name + /// Options for changing the API response + /// Thrown when a general API error occurs. + /// The list of s for the specified repository. + IObservable GetAll(string owner, string name, ApiOptions options); + /// /// Gets a single for the specified repository. /// diff --git a/Octokit.Reactive/Clients/ObservableReleasesClient.cs b/Octokit.Reactive/Clients/ObservableReleasesClient.cs index dfc9478e..100bd131 100644 --- a/Octokit.Reactive/Clients/ObservableReleasesClient.cs +++ b/Octokit.Reactive/Clients/ObservableReleasesClient.cs @@ -36,6 +36,26 @@ namespace Octokit.Reactive return _connection.GetAndFlattenAllPages(ApiUrls.Releases(owner, name)); } + /// + /// Gets all s for the specified repository. + /// + /// + /// See the API documentation for more information. + /// + /// The repository's owner + /// The repository's name + /// Options for changing the API response + /// Thrown when a general API error occurs. + /// The list of s for the specified repository. + public IObservable GetAll(string owner, string name, ApiOptions options) + { + Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); + Ensure.ArgumentNotNullOrEmptyString(name, "name"); + Ensure.ArgumentNotNull(options, "options"); + + return _connection.GetAndFlattenAllPages(ApiUrls.Releases(owner, name), options); + } + /// /// Gets a single for the specified repository. /// diff --git a/Octokit.Reactive/Helpers/ConnectionExtensions.cs b/Octokit.Reactive/Helpers/ConnectionExtensions.cs index 0094134c..63f8eb72 100644 --- a/Octokit.Reactive/Helpers/ConnectionExtensions.cs +++ b/Octokit.Reactive/Helpers/ConnectionExtensions.cs @@ -1,5 +1,7 @@ using System; using System.Collections.Generic; +using System.Globalization; +using System.Linq; using System.Reactive.Linq; using System.Reactive.Threading.Tasks; @@ -12,6 +14,26 @@ namespace Octokit.Reactive.Internal return GetPages(url, null, (pageUrl, pageParams) => connection.Get>(pageUrl, null, null).ToObservable()); } + public static IObservable GetAndFlattenAllPages(this IConnection connection, Uri url, ApiOptions options) + { + return GetPagesWithOptions(url, options, (pageUrl, o) => + { + var parameters = new Dictionary(); + + if (options.PageSize.HasValue) + { + parameters.Add("per_page", options.PageSize.Value.ToString(CultureInfo.InvariantCulture)); + } + + if (options.StartPage.HasValue) + { + parameters.Add("page", options.StartPage.Value.ToString(CultureInfo.InvariantCulture)); + } + + return connection.Get>(pageUrl, parameters, null).ToObservable(); + }); + } + public static IObservable GetAndFlattenAllPages(this IConnection connection, Uri url, IDictionary parameters) { return GetPages(url, parameters, (pageUrl, pageParams) => connection.Get>(pageUrl, pageParams, null).ToObservable()); @@ -35,5 +57,60 @@ namespace Octokit.Reactive.Internal .Where(resp => resp != null) .SelectMany(resp => resp.Body); } + + static IObservable GetPagesWithOptions(Uri uri, ApiOptions options, + Func>>> getPageFunc) + { + return getPageFunc(uri, options).Expand(resp => + { + var nextPageUri = resp.HttpResponse.ApiInfo.GetNextPageUrl(); + + if (nextPageUri == null) + { + return Observable.Empty>>(); + } + + if (nextPageUri.Query.Contains("page=") && options.PageCount.HasValue) + { + var allValues = ToQueryStringDictionary(nextPageUri); + + string pageValue; + if (allValues.TryGetValue("page", out pageValue)) + { + var startPage = options.StartPage ?? 1; + var pageCount = options.PageCount.Value; + + var endPage = startPage + pageCount; + if (pageValue.Equals(endPage.ToString(CultureInfo.InvariantCulture), StringComparison.OrdinalIgnoreCase)) + { + return Observable.Empty>>(); + } + } + } + + return Observable.Defer(() => getPageFunc(nextPageUri, null)); + }) + .Where(resp => resp != null) + .SelectMany(resp => resp.Body); + } + + static Dictionary ToQueryStringDictionary(Uri uri) + { + return uri.Query.Split('&') + .Select(keyValue => + { + var indexOf = keyValue.IndexOf('='); + if (indexOf > 0) + { + var key = keyValue.Substring(0, indexOf); + var value = keyValue.Substring(indexOf + 1); + return new KeyValuePair(key, value); + } + + //just a plain old value, return it + return new KeyValuePair(keyValue, null); + }) + .ToDictionary(x => x.Key, x => x.Value); + } } } diff --git a/Octokit.Tests.Integration/Octokit.Tests.Integration.csproj b/Octokit.Tests.Integration/Octokit.Tests.Integration.csproj index d8df0040..563b5313 100644 --- a/Octokit.Tests.Integration/Octokit.Tests.Integration.csproj +++ b/Octokit.Tests.Integration/Octokit.Tests.Integration.csproj @@ -132,6 +132,7 @@ + diff --git a/Octokit.Tests.Integration/Reactive/ObservableReleaseClientTests.cs b/Octokit.Tests.Integration/Reactive/ObservableReleaseClientTests.cs new file mode 100644 index 00000000..337f2894 --- /dev/null +++ b/Octokit.Tests.Integration/Reactive/ObservableReleaseClientTests.cs @@ -0,0 +1,88 @@ +using System.Reactive.Linq; +using System.Threading.Tasks; +using Octokit.Reactive; +using Xunit; + +namespace Octokit.Tests.Integration.Reactive +{ + public class ObservableReleaseClientTests + { + public class TheGetAllMethod + { + readonly ObservableReleasesClient _releaseClient; + const string owner = "octokit"; + const string name = "octokit.net"; + + public TheGetAllMethod() + { + var github = Helper.GetAuthenticatedClient(); + + _releaseClient = new ObservableReleasesClient(github); + } + + [IntegrationTest] + public async Task ReturnsReleases() + { + var releases = await _releaseClient.GetAll(owner, name).ToList(); + + Assert.NotEmpty(releases); + } + + [IntegrationTest] + public async Task ReturnsCorrectCountOfReleasesWithoutStart() + { + var options = new ApiOptions + { + PageSize = 5, + PageCount = 1 + }; + + var releases = await _releaseClient.GetAll(owner, name, options).ToList(); + + Assert.Equal(5, releases.Count); + } + + [IntegrationTest] + public async Task ReturnsCorrectCountOfReleasesWithStart() + { + var options = new ApiOptions + { + PageSize = 5, + PageCount = 1, + StartPage = 2 + }; + + var releases = await _releaseClient.GetAll(owner, name, options).ToList(); + + Assert.Equal(5, releases.Count); + } + + [IntegrationTest] + public async Task ReturnsDistinctResultsBasedOnStartPage() + { + var startOptions = new ApiOptions + { + PageSize = 5, + PageCount = 1 + }; + + var firstPage = await _releaseClient.GetAll(owner, name, startOptions).ToList(); + + var skipStartOptions = new ApiOptions + { + PageSize = 5, + PageCount = 1, + StartPage = 2 + }; + + var secondPage = await _releaseClient.GetAll(owner, name, skipStartOptions).ToList(); + + Assert.NotEqual(firstPage[0].Id, secondPage[0].Id); + Assert.NotEqual(firstPage[1].Id, secondPage[1].Id); + Assert.NotEqual(firstPage[2].Id, secondPage[2].Id); + Assert.NotEqual(firstPage[3].Id, secondPage[3].Id); + Assert.NotEqual(firstPage[4].Id, secondPage[4].Id); + } + } + } +} From b41c00c4c450bbae4f48447db1bcc67697e78e48 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 18:04:28 +1100 Subject: [PATCH 11/16] extracted pagination function to separate type --- Octokit.Reactive/Octokit.Reactive-Mono.csproj | 5 +- .../Octokit.Reactive-MonoAndroid.csproj | 5 +- .../Octokit.Reactive-Monotouch.csproj | 5 +- Octokit.Reactive/Octokit.Reactive.csproj | 3 + Octokit/Helpers/Pagination.cs | 76 +++++++++++++++++++ Octokit/Octokit-Mono.csproj | 1 + Octokit/Octokit-MonoAndroid.csproj | 1 + Octokit/Octokit-Monotouch.csproj | 1 + Octokit/Octokit-Portable.csproj | 1 + Octokit/Octokit-netcore45.csproj | 1 + Octokit/Octokit.csproj | 3 +- 11 files changed, 98 insertions(+), 4 deletions(-) create mode 100644 Octokit/Helpers/Pagination.cs diff --git a/Octokit.Reactive/Octokit.Reactive-Mono.csproj b/Octokit.Reactive/Octokit.Reactive-Mono.csproj index 10d63457..80fd88f2 100644 --- a/Octokit.Reactive/Octokit.Reactive-Mono.csproj +++ b/Octokit.Reactive/Octokit.Reactive-Mono.csproj @@ -52,6 +52,9 @@ Helpers\Ensure.cs + + Helpers\Pagination.cs + Properties\SolutionInfo.cs @@ -179,4 +182,4 @@ Octokit-Mono - \ No newline at end of file + diff --git a/Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj b/Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj index d0f878da..ce887c4c 100644 --- a/Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj +++ b/Octokit.Reactive/Octokit.Reactive-MonoAndroid.csproj @@ -60,6 +60,9 @@ Helpers\Ensure.cs + + Helpers\Pagination.cs + Properties\SolutionInfo.cs @@ -187,4 +190,4 @@ Octokit-MonoAndroid - \ No newline at end of file + diff --git a/Octokit.Reactive/Octokit.Reactive-Monotouch.csproj b/Octokit.Reactive/Octokit.Reactive-Monotouch.csproj index ce3efdbe..14a1c7ea 100644 --- a/Octokit.Reactive/Octokit.Reactive-Monotouch.csproj +++ b/Octokit.Reactive/Octokit.Reactive-Monotouch.csproj @@ -56,6 +56,9 @@ Helpers\Ensure.cs + + Helpers\Pagination.cs + Properties\SolutionInfo.cs @@ -183,4 +186,4 @@ Octokit-Monotouch - \ No newline at end of file + diff --git a/Octokit.Reactive/Octokit.Reactive.csproj b/Octokit.Reactive/Octokit.Reactive.csproj index 607a71eb..ebf316aa 100644 --- a/Octokit.Reactive/Octokit.Reactive.csproj +++ b/Octokit.Reactive/Octokit.Reactive.csproj @@ -72,6 +72,9 @@ Helpers\Ensure.cs + + Helpers\Pagination.cs + Properties\SolutionInfo.cs diff --git a/Octokit/Helpers/Pagination.cs b/Octokit/Helpers/Pagination.cs new file mode 100644 index 00000000..1acf39ea --- /dev/null +++ b/Octokit/Helpers/Pagination.cs @@ -0,0 +1,76 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; + +namespace Octokit +{ + internal static class Pagination + { + internal static IDictionary Setup(IDictionary parameters, ApiOptions options) + { + parameters = parameters ?? new Dictionary(); + + if (options.PageSize.HasValue) + { + parameters.Add("per_page", options.PageSize.Value.ToString(CultureInfo.InvariantCulture)); + } + + if (options.StartPage.HasValue) + { + parameters.Add("page", options.StartPage.Value.ToString(CultureInfo.InvariantCulture)); + } + + return parameters; + } + + internal static bool ShouldContinue( + Uri uri, + ApiOptions options) + { + if (uri == null) + { + return false; + } + + if (uri.Query.Contains("page=") && options.PageCount.HasValue) + { + var allValues = ToQueryStringDictionary(uri); + + string pageValue; + if (allValues.TryGetValue("page", out pageValue)) + { + var startPage = options.StartPage ?? 1; + var pageCount = options.PageCount.Value; + + var endPage = startPage + pageCount; + if (pageValue.Equals(endPage.ToString(CultureInfo.InvariantCulture), StringComparison.OrdinalIgnoreCase)) + { + return false; + } + } + } + + return true; + } + + static Dictionary ToQueryStringDictionary(Uri uri) + { + return uri.Query.Split('&') + .Select(keyValue => + { + var indexOf = keyValue.IndexOf('='); + if (indexOf > 0) + { + var key = keyValue.Substring(0, indexOf); + var value = keyValue.Substring(indexOf + 1); + return new KeyValuePair(key, value); + } + + //just a plain old value, return it + return new KeyValuePair(keyValue, null); + }) + .ToDictionary(x => x.Key, x => x.Value); + } + } +} diff --git a/Octokit/Octokit-Mono.csproj b/Octokit/Octokit-Mono.csproj index c518b97d..3e4bcc99 100644 --- a/Octokit/Octokit-Mono.csproj +++ b/Octokit/Octokit-Mono.csproj @@ -448,6 +448,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-MonoAndroid.csproj b/Octokit/Octokit-MonoAndroid.csproj index 904a34c0..4bda9bba 100644 --- a/Octokit/Octokit-MonoAndroid.csproj +++ b/Octokit/Octokit-MonoAndroid.csproj @@ -456,6 +456,7 @@ + \ No newline at end of file diff --git a/Octokit/Octokit-Monotouch.csproj b/Octokit/Octokit-Monotouch.csproj index 9816e046..8dabb0fa 100644 --- a/Octokit/Octokit-Monotouch.csproj +++ b/Octokit/Octokit-Monotouch.csproj @@ -452,6 +452,7 @@ + diff --git a/Octokit/Octokit-Portable.csproj b/Octokit/Octokit-Portable.csproj index f2faa095..6f03fac3 100644 --- a/Octokit/Octokit-Portable.csproj +++ b/Octokit/Octokit-Portable.csproj @@ -445,6 +445,7 @@ + diff --git a/Octokit/Octokit-netcore45.csproj b/Octokit/Octokit-netcore45.csproj index 2a4d4aa8..7268c43b 100644 --- a/Octokit/Octokit-netcore45.csproj +++ b/Octokit/Octokit-netcore45.csproj @@ -452,6 +452,7 @@ + diff --git a/Octokit/Octokit.csproj b/Octokit/Octokit.csproj index 22536261..0eb8c1d0 100644 --- a/Octokit/Octokit.csproj +++ b/Octokit/Octokit.csproj @@ -102,6 +102,7 @@ + @@ -484,4 +485,4 @@ --> - + \ No newline at end of file From b97254c5a715589dce61645274f0059cba3c4b04 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Sun, 14 Feb 2016 18:04:51 +1100 Subject: [PATCH 12/16] swap in shared component for existing rules --- .../Helpers/ConnectionExtensions.cs | 61 +++---------------- Octokit/Http/ApiConnection.cs | 55 +++-------------- 2 files changed, 14 insertions(+), 102 deletions(-) diff --git a/Octokit.Reactive/Helpers/ConnectionExtensions.cs b/Octokit.Reactive/Helpers/ConnectionExtensions.cs index 63f8eb72..becbf01c 100644 --- a/Octokit.Reactive/Helpers/ConnectionExtensions.cs +++ b/Octokit.Reactive/Helpers/ConnectionExtensions.cs @@ -18,18 +18,7 @@ namespace Octokit.Reactive.Internal { return GetPagesWithOptions(url, options, (pageUrl, o) => { - var parameters = new Dictionary(); - - if (options.PageSize.HasValue) - { - parameters.Add("per_page", options.PageSize.Value.ToString(CultureInfo.InvariantCulture)); - } - - if (options.StartPage.HasValue) - { - parameters.Add("page", options.StartPage.Value.ToString(CultureInfo.InvariantCulture)); - } - + var parameters = Pagination.Setup(new Dictionary(), options); return connection.Get>(pageUrl, parameters, null).ToObservable(); }); } @@ -65,52 +54,16 @@ namespace Octokit.Reactive.Internal { var nextPageUri = resp.HttpResponse.ApiInfo.GetNextPageUrl(); - if (nextPageUri == null) - { - return Observable.Empty>>(); - } + var shouldContinue = Pagination.ShouldContinue( + nextPageUri, + options); - if (nextPageUri.Query.Contains("page=") && options.PageCount.HasValue) - { - var allValues = ToQueryStringDictionary(nextPageUri); - - string pageValue; - if (allValues.TryGetValue("page", out pageValue)) - { - var startPage = options.StartPage ?? 1; - var pageCount = options.PageCount.Value; - - var endPage = startPage + pageCount; - if (pageValue.Equals(endPage.ToString(CultureInfo.InvariantCulture), StringComparison.OrdinalIgnoreCase)) - { - return Observable.Empty>>(); - } - } - } - - return Observable.Defer(() => getPageFunc(nextPageUri, null)); + return shouldContinue + ? Observable.Defer(() => getPageFunc(nextPageUri, null)) + : Observable.Empty>>(); }) .Where(resp => resp != null) .SelectMany(resp => resp.Body); } - - static Dictionary ToQueryStringDictionary(Uri uri) - { - return uri.Query.Split('&') - .Select(keyValue => - { - var indexOf = keyValue.IndexOf('='); - if (indexOf > 0) - { - var key = keyValue.Substring(0, indexOf); - var value = keyValue.Substring(indexOf + 1); - return new KeyValuePair(key, value); - } - - //just a plain old value, return it - return new KeyValuePair(keyValue, null); - }) - .ToDictionary(x => x.Key, x => x.Value); - } } } diff --git a/Octokit/Http/ApiConnection.cs b/Octokit/Http/ApiConnection.cs index 179d898d..818cd941 100644 --- a/Octokit/Http/ApiConnection.cs +++ b/Octokit/Http/ApiConnection.cs @@ -182,17 +182,7 @@ namespace Octokit Ensure.ArgumentNotNull(uri, "uri"); Ensure.ArgumentNotNull(options, "options"); - parameters = parameters ?? new Dictionary(); - - if (options.PageSize.HasValue) - { - parameters.Add("per_page", options.PageSize.Value.ToString(CultureInfo.InvariantCulture)); - } - - if (options.StartPage.HasValue) - { - parameters.Add("page", options.StartPage.Value.ToString(CultureInfo.InvariantCulture)); - } + parameters = Pagination.Setup(parameters, options); return _pagination.GetAllPages(async () => await GetPage(uri, parameters, accepts, options) .ConfigureAwait(false), uri); @@ -549,45 +539,14 @@ namespace Octokit response, nextPageUri => { - if (nextPageUri.Query.Contains("page=") && options.PageCount.HasValue) - { - var allValues = ToQueryStringDictionary(nextPageUri); + var shouldContinue = Pagination.ShouldContinue( + nextPageUri, + options); - string pageValue; - if (allValues.TryGetValue("page", out pageValue)) - { - var startPage = options.StartPage ?? 1; - var pageCount = options.PageCount.Value; - - var endPage = startPage + pageCount; - if (pageValue.Equals(endPage.ToString(), StringComparison.OrdinalIgnoreCase)) - { - return null; - } - } - } - - return connection.Get>(nextPageUri, parameters, accepts); + return shouldContinue + ? connection.Get>(nextPageUri, parameters, accepts) + : null; }); } - - static Dictionary ToQueryStringDictionary(Uri uri) - { - return uri.Query.Split('&') - .Select(keyValue => - { - var indexOf = keyValue.IndexOf('='); - if (indexOf > 0) - { - var key = keyValue.Substring(0, indexOf); - var value = keyValue.Substring(indexOf + 1); - return new KeyValuePair(key, value); - } - - //just a plain old value, return it - return new KeyValuePair(keyValue, null); - }) - .ToDictionary(x => x.Key, x => x.Value); - } } } From e27b58d043bd10e5d1ea6844a8b64dca6cb2d3f7 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Wed, 17 Feb 2016 11:14:39 +1100 Subject: [PATCH 13/16] a bit of cleanup --- Octokit.Tests.Conventions/PaginationTests.cs | 2 +- Octokit.Tests/Clients/ReleasesClientTests.cs | 2 +- Octokit/Http/ApiConnection.cs | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Octokit.Tests.Conventions/PaginationTests.cs b/Octokit.Tests.Conventions/PaginationTests.cs index fcd76b63..3c2565e7 100644 --- a/Octokit.Tests.Conventions/PaginationTests.cs +++ b/Octokit.Tests.Conventions/PaginationTests.cs @@ -8,7 +8,7 @@ namespace Octokit.Tests.Conventions { public class PaginationTests { - [Theory(Skip = "Disable this to run it and find all the places where things break")] + [Theory(Skip = "Enable this to run it and find all the places where things break")] [MemberData("GetClientInterfaces")] public void CheckObservableClients(Type clientInterface) { diff --git a/Octokit.Tests/Clients/ReleasesClientTests.cs b/Octokit.Tests/Clients/ReleasesClientTests.cs index bb0ef316..d82c0707 100644 --- a/Octokit.Tests/Clients/ReleasesClientTests.cs +++ b/Octokit.Tests/Clients/ReleasesClientTests.cs @@ -20,7 +20,7 @@ namespace Octokit.Tests.Clients client.Received().GetAll(Arg.Is(u => u.ToString() == "repos/fake/repo/releases"), null, - AcceptHeaders.StableVersion, + "application/vnd.github.v3", Args.ApiOptions); } diff --git a/Octokit/Http/ApiConnection.cs b/Octokit/Http/ApiConnection.cs index 818cd941..2805ff29 100644 --- a/Octokit/Http/ApiConnection.cs +++ b/Octokit/Http/ApiConnection.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; -using System.Globalization; -using System.Linq; using System.Net; using System.Threading; using System.Threading.Tasks; From b036bb931b291ab72a9b84bb557a5e20b132477d Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 18 Feb 2016 14:03:48 +1100 Subject: [PATCH 14/16] corrected this test to be 1-based (and trigger null) --- Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs b/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs index 6b690a63..908ff7e7 100644 --- a/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs +++ b/Octokit.Tests/Models/ReadOnlyPagedCollectionTests.cs @@ -77,7 +77,7 @@ namespace Octokit.Tests.Models connection.Get>(nextPageUrl, null, null).Returns(nextPageResponse); - var pageCount = 0; + var pageCount = 1; var pagedCollection = new ReadOnlyPagedCollection( response, @@ -95,7 +95,7 @@ namespace Octokit.Tests.Models var second = await pagedCollection.GetNextPage(); Assert.NotNull(first); - Assert.NotNull(second); + Assert.Null(second); } } } From e93dc53e8c234f0c4f44b35bbc9f7de0648b5346 Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Thu, 18 Feb 2016 14:04:51 +1100 Subject: [PATCH 15/16] less nulls in your life --- Octokit.Tests/Clients/AuthorizationsClientTests.cs | 2 +- Octokit/Clients/AuthorizationsClient.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit.Tests/Clients/AuthorizationsClientTests.cs b/Octokit.Tests/Clients/AuthorizationsClientTests.cs index 1a05b4f4..5aad42ef 100644 --- a/Octokit.Tests/Clients/AuthorizationsClientTests.cs +++ b/Octokit.Tests/Clients/AuthorizationsClientTests.cs @@ -35,7 +35,7 @@ namespace Octokit.Tests.Clients client.Received().GetAll( Arg.Is(u => u.ToString() == "authorizations"), - Arg.Is>(d => d == null)); + Args.ApiOptions); } } diff --git a/Octokit/Clients/AuthorizationsClient.cs b/Octokit/Clients/AuthorizationsClient.cs index e7fd862e..84b41b78 100644 --- a/Octokit/Clients/AuthorizationsClient.cs +++ b/Octokit/Clients/AuthorizationsClient.cs @@ -36,7 +36,7 @@ namespace Octokit /// A list of s. public Task> GetAll() { - return ApiConnection.GetAll(ApiUrls.Authorizations(), (IDictionary)null); + return ApiConnection.GetAll(ApiUrls.Authorizations(), ApiOptions.None); } /// From 03a5542fe6f281ae991a6fdd818b24d2eb80c74a Mon Sep 17 00:00:00 2001 From: Brendan Forster Date: Wed, 24 Feb 2016 11:35:10 +1100 Subject: [PATCH 16/16] :lipstick: renaming --- .../Exception/ApiOptionsMissingException.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs b/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs index f913ab94..61d1111f 100644 --- a/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs +++ b/Octokit.Tests.Conventions/Exception/ApiOptionsMissingException.cs @@ -21,10 +21,10 @@ namespace Octokit.Tests.Conventions static string FormatMethod(MethodInfo m) { - var parametersFormatteds = m.GetParameters() + var formattedParameters = m.GetParameters() .Select(p => string.Format("{0} {1}", p.ParameterType.Name, p.Name)); - var parameterList = string.Join(", ", parametersFormatteds); + var parameterList = string.Join(", ", formattedParameters); return string.Format(" - {0}({1})", m.Name, parameterList); }