Implement Statistics api #296

ammeep · 2014-01-09T21:58:41Z

Fixes #294

💃

Todo

Implement get contributors list
Implement get commit activity data
Implement get number of additions and deletions per week
Implement get weekly commit count
Implement get commits per hour
Handle 202, 200 for queued jobs
Tests

haacked · 2014-01-10T04:28:50Z

Octokit/Clients/StatisticsClient.cs

+        /// <param name="owner">The owner of the repository</param>
+        /// <param name="repoName">The name of the repository</param>
+        /// <returns>A list of <see cref="Contributor"/></returns>
+        public Task<IEnumerable<Contributor>> Contributors(string owner, string repoName)


What is a "repo". 😛 We generally use name. If it's not clear that it's a repository, I'd prefer repositoryName

haacked · 2014-01-10T04:31:28Z

It's a good start!

ammeep · 2014-01-11T01:38:18Z

These set of API's are a little different to what we already have in that the operations are expensive. On the first request the calculation of the stats is queued up and you are returned a 202. You retry the request until the job is complete and a 200 is returned.

Should Get be responsible for tracking the 202, and 200. Or do we provide a pair of operations - one to queue the job, and another to wait for its completion?

The downside with the latter - the 'wait for completion' method would still need to take into account that the stats might need to be recalculated server side (which will happen on each push to master)

I prefer the first option, but does anyone have any thoughts on how we want this to look on our client API?

haacked · 2014-01-11T05:08:58Z

I like the first option. With the observable API, we could have calling Dispose on the subscription cancel subsequent requests.

shiftkey · 2014-01-11T05:57:11Z

I prefer the first option because it takes away the pain of having to implement the "poll until you get a 200" logic - which could be implemented in wildly different ways by different developers causing all sorts of headaches...

pmacn · 2014-01-11T07:00:58Z

Just a suggestion. You might want to make sure that the client methods (the non-reactive ones, not sure how Rx deals with cancellation yet) have overloads that take cancellation tokens if the calls could take long enough that a client would want to cancel.

shiftkey · 2014-01-13T18:05:46Z

@pmacn 👍

ammeep · 2014-02-03T02:06:06Z

One question I have - this API is not exactly user friendly. It likes to use arrays of ints everywhere. I would be keen to add some smarts to the objects being returned so that these are useable. For example

Get the number of commits per hour in each day

Returns: [ [2, 14, 25], ......]
Which actually means:

Each array contains the day number, hour number, and number of commits:
0-6: Sunday - Saturday
0-23: Hour of day
Number of commits

What do people think if we beefed up the models being returned from the statistics client to make this a much more consumable/understandable object.

shiftkey · 2014-02-03T03:28:32Z

@ammeep .\build FixProjects to sync up the Mono* projects

shiftkey · 2014-02-03T03:39:07Z

It likes to use arrays of ints everywhere.

Yep, transforming this into a dataset that's easier to traverse is 💚 from me.

A quick way to get started would be to tailor the response based on the specific API you're calling, which seems like you're already on that path.

A crazy idea: encapsulate the data behind a more human-friendly API in the response.

public class PunchCardStats 
{
    public int GetTotal(DateTime pointInTime) { ... }
    public int GetTotal(DateTime startTime, TimeSpan timeSpan) { ... }
}

rather than exposing the array directly (well you could do that anyway)...

ammeep · 2014-02-03T04:18:59Z

.\build FixProjects to sync up the Mono* projects

😍

A crazy idea: encapsulate the data behind a more human-friendly API in the response. rather than exposing the array directly (well you could do that anyway)...

This is exactly what I was thinking 💥 love it. I'm not fussed on exposing the underlying array, we should be able to wing it so that people don't need this.

Not a fan of those suppress messages on weekly hash. Will see if we can clean this up 🔜

Also return a list of contributors because.reasons (being that is what the api returns)

This is crude I know, but will be revisited once all apis implemented

Left the rest of the connection api as is. I feel like all of these should support cancellation. But this can be done in another PR.

+ test fix up and test of cancellation.

💄

ammeep · 2014-02-16T05:35:17Z

Anyone keen to give this puppy a review? It seems there are a bunch of tests in master that are also broken in this branch. Debugger displays missing etc - I fixed up those added in this PR. Apart from that. Keen for feedback 👍

shiftkey · 2014-02-18T00:00:55Z

Octokit/Http/Connection.cs

            _jsonPipeline.DeserializeResponse(response);
            return response;
        }

        // THIS IS THE METHOD THAT EVERY REQUEST MUST GO THROUGH!
-        async Task<IResponse<T>> RunRequest<T>(IRequest request)
+        async Task<IResponse<T>> RunRequest<T>(IRequest request, CancellationToken cancellationToken)


We should pass this cancellation token into _httpClient.Send<T> to make the FxCop warning go away...

Also that was the entire point of this change. My bad.

shiftkey · 2014-02-18T00:16:47Z

Got those tests passing, now reading it a bit more closely

shiftkey · 2014-02-18T00:19:29Z

Octokit/Http/JsonHttpPipeline.cs

@@ -44,8 +44,12 @@ public void DeserializeResponse<T>(IResponse<T> response)

            if (response.ContentType != null && response.ContentType.Equals("application/json", StringComparison.Ordinal))
            {
-                var json = _serializer.Deserialize<T>(response.Body);
-                response.BodyAsObject = json;
+                // simple json does not support the root node being empty. Will submit a pr but in the mean time....


FYI: https://github.com/facebook-csharp-sdk/simple-json is where to go for this

cc @prabirshrestha

shiftkey · 2014-02-18T00:32:17Z

A few little places in the new tests where parameters are crammed together, but I'm definitely into whitespace OCD now in terms of reviewing.

shiftkey · 2014-02-18T00:34:40Z

I'm happy to merge this in as a first pass for the feature - we can refine the data we return to the user in the future based on what users would like.

Ping me when you're happy with the whitespace stuff and I'll

ammeep · 2014-02-18T03:21:03Z

we can refine the data we return to the user in the future based on what users would like.

👍

@shiftkey ping... I'm happy with this now.

Implement Statistics api

haacked reviewed Jan 10, 2014
View reviewed changes

hnrkndrssn mentioned this pull request Jan 21, 2014

What's Left To Implement? #246

Closed

ammeep added 16 commits February 16, 2014 17:03

Added stats/contributors client & response objects

92d3d18

Not a fan of those suppress messages on weekly hash. Will see if we can clean this up 🔜

Stubbed implementation of IStatisticsClient

91b0592

Updated signature to include repo name and owner

af4c82a

Also return a list of contributors because.reasons (being that is what the api returns)

renamed parameter

98afca5

Implement client

feb52c0

Test stats client get contributors

29461db

💄 renamed to GetContributors

23ee959

Added Statistics Client

f926187

💄

c14acda

ObservableStats Client

fd71335

Observable Client tests

6a66bf9

Added intergration tests for contributors api

60fc710

Retry until call is successful

021ef3d

This is crude I know, but will be revisited once all apis implemented

a work around for simple json support for empty root nodes

82fcea6

Get commit activity for the last year

815da31

Simple intergration test for commit activity

c03626c

ammeep added 14 commits February 16, 2014 17:04

Added is any cancellation token argument

2e1fcd7

GetAsync<T> overload with cancellation token

a1e62ec

Left the rest of the connection api as is. I feel like all of these should support cancellation. But this can be done in another PR.

GetQueuedOperation<T> takes cancellation token

e71c3e3

+ test fix up and test of cancellation.

Fix up mono projects

a975e77

Provide cancellation token option

63da057

👊 card statistics 2.0

ec5db2c

Line up with the api - what is this all about

ddab946

💄

💄 Lets talk about code frequencies

5a168f0

💄 for commit activities

7102921

Unix Timestamp helper

8469a50

Because A D and C is meaningful....

0a01e4c

fixed indenting

dd15b62

💄

c912825

Debugger displays

1a78ed7

shiftkey reviewed Feb 18, 2014
View reviewed changes

shiftkey added 3 commits February 18, 2014 11:03

use the new cancellation token

668f79d

updated tests to use correct mock

c3cf8d1

removed redundant Send<T> from IHttpClient

5f97d10

shiftkey reviewed Feb 18, 2014
View reviewed changes

ammeep added 2 commits February 18, 2014 16:01

💄

66c19ad

Merge branch 'master' into ammeep/statistics-api

36427ea

shiftkey added a commit that referenced this pull request Feb 18, 2014

Merge pull request #296 from octokit/ammeep/statistics-api

caa6bd7

Implement Statistics api

shiftkey merged commit caa6bd7 into master Feb 18, 2014

shiftkey deleted the ammeep/statistics-api branch February 18, 2014 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Statistics api #296

Implement Statistics api #296

ammeep commented Jan 9, 2014

haacked Jan 10, 2014

haacked commented Jan 10, 2014

ammeep commented Jan 11, 2014

haacked commented Jan 11, 2014

shiftkey commented Jan 11, 2014

pmacn commented Jan 11, 2014

shiftkey commented Jan 13, 2014

ammeep commented Feb 3, 2014

shiftkey commented Feb 3, 2014

shiftkey commented Feb 3, 2014

ammeep commented Feb 3, 2014

ammeep commented Feb 16, 2014

shiftkey Feb 18, 2014

ammeep Feb 18, 2014

shiftkey commented Feb 18, 2014

shiftkey Feb 18, 2014

shiftkey commented Feb 18, 2014

shiftkey commented Feb 18, 2014

ammeep commented Feb 18, 2014

Implement Statistics api #296

Implement Statistics api #296

Conversation

ammeep commented Jan 9, 2014

Todo

haacked Jan 10, 2014

Choose a reason for hiding this comment

haacked commented Jan 10, 2014

ammeep commented Jan 11, 2014

haacked commented Jan 11, 2014

shiftkey commented Jan 11, 2014

pmacn commented Jan 11, 2014

shiftkey commented Jan 13, 2014

ammeep commented Feb 3, 2014

Get the number of commits per hour in each day

shiftkey commented Feb 3, 2014

shiftkey commented Feb 3, 2014

ammeep commented Feb 3, 2014

ammeep commented Feb 16, 2014

shiftkey Feb 18, 2014

Choose a reason for hiding this comment

ammeep Feb 18, 2014

Choose a reason for hiding this comment

shiftkey commented Feb 18, 2014

shiftkey Feb 18, 2014

Choose a reason for hiding this comment

shiftkey commented Feb 18, 2014

shiftkey commented Feb 18, 2014

ammeep commented Feb 18, 2014