-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Reactive Milestone Client #138
Merged
Merged
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
5baaa17
Add optional GetAndFlattenAllPages() parameters
dahlbyk 9b5b00b
Add and implement IObservableMilestonesClient
dahlbyk e6ec265
Update IMilestoneClient docs
dahlbyk 854c6af
Don't use parameters beyond first page
dahlbyk 864c97a
Add missing namespaces
dahlbyk 77e18f5
MilestonesClientTests shouldn't test IssuesClient
dahlbyk e549376
Add ObservableMilestoneClient unit tests
dahlbyk 9c1ace9
Add ObservableMilestoneClient int tests
dahlbyk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Reactive; | ||
|
|
||
| namespace Octokit | ||
| { | ||
| public interface IObservableMilestonesClient | ||
| { | ||
| /// <summary> | ||
| /// Gets a single Milestone by number. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#get-a-single-milestone | ||
| /// </remarks> | ||
| /// <returns></returns> | ||
| [SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Get", | ||
| Justification = "Method makes a network request")] | ||
| IObservable<Milestone> Get(string owner, string name, int number); | ||
|
|
||
| /// <summary> | ||
| /// Gets all open milestones for the repository. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#list-milestones-for-a-repository | ||
| /// </remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <returns></returns> | ||
| IObservable<Milestone> GetForRepository(string owner, string name); | ||
|
|
||
| /// <summary> | ||
| /// Gets all open milestones for the repository. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#list-milestones-for-a-repository | ||
| /// </remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="request">Used to filter and sort the list of Milestones returned</param> | ||
| /// <returns></returns> | ||
| IObservable<Milestone> GetForRepository(string owner, string name, MilestoneRequest request); | ||
|
|
||
| /// <summary> | ||
| /// Creates a milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#create-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="newMilestone">A <see cref="NewMilestone"/> instance describing the new Milestone to create</param> | ||
| /// <returns></returns> | ||
| IObservable<Milestone> Create(string owner, string name, NewMilestone newMilestone); | ||
|
|
||
| /// <summary> | ||
| /// Creates a milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#update-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="number">The Milestone number</param> | ||
| /// <param name="milestoneUpdate">An <see cref="MilestoneUpdate"/> instance describing the changes to make to the Milestone | ||
| /// </param> | ||
| /// <returns></returns> | ||
| IObservable<Milestone> Update(string owner, string name, int number, MilestoneUpdate milestoneUpdate); | ||
|
|
||
| /// <summary> | ||
| /// Deletes a milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#delete-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="number">The milestone number</param> | ||
| /// <returns></returns> | ||
| IObservable<Unit> Delete(string owner, string name, int number); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| using System; | ||
| using System.Reactive; | ||
| using System.Reactive.Threading.Tasks; | ||
| using Octokit.Reactive.Internal; | ||
|
|
||
| namespace Octokit.Reactive.Clients | ||
| { | ||
| public class ObservableMilestonesClient : IObservableMilestonesClient | ||
| { | ||
| readonly IMilestonesClient _client; | ||
| readonly IConnection _connection; | ||
|
|
||
| public ObservableMilestonesClient(IGitHubClient client) | ||
| { | ||
| Ensure.ArgumentNotNull(client, "client"); | ||
|
|
||
| _client = client.Issue.Milestone; | ||
| _connection = client.Connection; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a single Milestone by number. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#get-a-single-milestone | ||
| /// </remarks> | ||
| /// <returns></returns> | ||
| public IObservable<Milestone> Get(string owner, string name, int number) | ||
| { | ||
| Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); | ||
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
|
|
||
| return _client.Get(owner, name, number).ToObservable(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets all open milestones for the repository. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#list-milestones-for-a-repository | ||
| /// </remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <returns></returns> | ||
| public IObservable<Milestone> GetForRepository(string owner, string name) | ||
| { | ||
| return _connection.GetAndFlattenAllPages<Milestone>(ApiUrls.Milestones(owner, name)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets all open milestones for the repository. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// http://developer.github.com/v3/issues/milestones/#list-milestones-for-a-repository | ||
| /// </remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="request">Used to filter and sort the list of Milestones returned</param> | ||
| /// <returns></returns> | ||
| public IObservable<Milestone> GetForRepository(string owner, string name, MilestoneRequest request) | ||
| { | ||
| Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); | ||
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
| Ensure.ArgumentNotNull(request, "request"); | ||
|
|
||
| return _connection.GetAndFlattenAllPages<Milestone>(ApiUrls.Milestones(owner, name), | ||
| request.ToParametersDictionary()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates an Milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#create-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="newMilestone">A <see cref="NewMilestone"/> instance describing the new Milestone to create</param> | ||
| /// <returns></returns> | ||
| public IObservable<Milestone> Create(string owner, string name, NewMilestone newMilestone) | ||
| { | ||
| Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); | ||
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
| Ensure.ArgumentNotNull(newMilestone, "newMilestone"); | ||
|
|
||
| return _client.Create(owner, name, newMilestone).ToObservable(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates an Milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#create-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="number">The Milestone number</param> | ||
| /// <param name="milestoneUpdate">An <see cref="MilestoneUpdate"/> instance describing the changes to make to the Milestone | ||
| /// </param> | ||
| /// <returns></returns> | ||
| public IObservable<Milestone> Update(string owner, string name, int number, MilestoneUpdate milestoneUpdate) | ||
| { | ||
| Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); | ||
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
| Ensure.ArgumentNotNull(milestoneUpdate, "milestoneUpdate"); | ||
|
|
||
| return _client.Update(owner, name, number, milestoneUpdate).ToObservable(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Deletes a milestone for the specified repository. Any user with pull access to a repository can create an | ||
| /// Milestone. | ||
| /// </summary> | ||
| /// <remarks>http://developer.github.com/v3/issues/milestones/#delete-a-milestone</remarks> | ||
| /// <param name="owner">The owner of the repository</param> | ||
| /// <param name="name">The name of the repository</param> | ||
| /// <param name="number">The milestone number</param> | ||
| /// <returns></returns> | ||
| public IObservable<Unit> Delete(string owner, string name, int number) | ||
| { | ||
| Ensure.ArgumentNotNullOrEmptyString(owner, "owner"); | ||
| Ensure.ArgumentNotNullOrEmptyString(name, "name"); | ||
|
|
||
| return _client.Delete(owner, name, number).ToObservable(); | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the
parametersis needed. ThenextPageUrlis returned as part of the previous page's response in theLinkheader. I would assume it would be a fully formed URL for retrieving the next page of results. See the pagination docs for the GitHub API for more details. Were you finding that this was not the case?The
acceptsparameter, on the other hand, makes total sense!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me take a closer look at this - I'm pretty sure we need
parametersfor the initial call but you're probably right that it's redundant for successive calls. Assuming that's the case, how does this look?public static IObservable<T> GetAndFlattenAllPages<T>(this IConnection connection, Uri url, IDictionary<string, string> parameters = null, string accepts = null) { - return GetPages(url, nextPageUrl => connection.GetAsync<List<T>>(nextPageUrl, parameters, accepts).ToObservable()); + return GetPages(url, parameters, (pageUrl, pageParams) => connection.GetAsync<List<T>>(pageUrl, pageParams, accepts).ToObservable()); } - static IObservable<T> GetPages<T>(Uri uri, - Func<Uri, IObservable<IResponse<List<T>>>> getPageFunc) + static IObservable<T> GetPages<T>(Uri uri, IDictionary<string, string> parameters, + Func<Uri, IDictionary<string, string>, IObservable<IResponse<List<T>>>> getPageFunc) { - return getPageFunc(uri).Expand(resp => + return getPageFunc(uri, parameters).Expand(resp => { var nextPageUrl = resp.ApiInfo.GetNextPageUrl(); return nextPageUrl == null ? Observable.Empty<IResponse<List<T>>>() - : Observable.Defer(() => getPageFunc(nextPageUrl)); + : Observable.Defer(() => getPageFunc(nextPageUrl, null)); }) .Where(resp => resp != null) .SelectMany(resp => resp.BodyAsObject);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, don't we already apply the parameters to the URL before we call this? If we do that, we never need to worry about it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is our only chance to apply parameters - without passing them in here, they're not respected. I don't suppose you or your colleagues know of a public repo with more than a page worth of Milestones for testing purposes? At least for the first page, this does correctly apply parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right. Why'd you remove the
acceptsparameter? That seems like it should be necessary if we need to pass something other than the default. Or do we not need to do that here?I don't offhand. I suppose we could create one. We could have the integration test create one. By default, it's 30 a page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MilestonesClientdoesn't passaccepts, so I'm not either. I believe it's still correctly wired up inGetAndFlattenAllPagesthough.