Reactive Milestone Client

[dahlbyk]

Using optional parameters in 5baaa17 seems cool since it's internal only.

[dahlbyk]

Prereq for #126

[haacked]

I'm not sure the parameters is needed. The nextPageUrl is returned as part of the previous page's response in the Link header. 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 accepts parameter, on the other hand, makes total sense!

[dahlbyk]

Let me take a closer look at this - I'm pretty sure we need parameters for 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);

[haacked]

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.

[dahlbyk]

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.

[haacked]

This is our only chance to apply parameters - without passing them in here, they're not respected.

Ah right. Why'd you remove the accepts parameter? 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?

know of a public repo with more than a page worth of Milestones for testing purposes?

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.

[dahlbyk]

Ah right. Why'd you remove the accepts parameter? 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?

MilestonesClient doesn't pass accepts, so I'm not either. I believe it's still correctly wired up in GetAndFlattenAllPages though.

[haacked]

Thanks a lot for submitting this! I had one comment on something that looks incorrect to me.

Also, would love to see some unit tests (or at the very least, an integration test).

[dahlbyk]

Now with ∞ more tests, though no integration test that exercises paging.

[haacked]

Thanks!