Added GetRateLimits to MiscellaneousClient #848

Red-Folder · 2015-07-24T20:53:47Z

Added support for the rate_limit API (https://developer.github.com/v3/rate_limit/). Note that this is the Miscellaneous API call - not the rate details returned as part of the headers for every API call. This functionality is referenced in #811

I'm interested in feedback (thus marked as WIP). Taken a couple of design decisions that may not fit with the overall solution:

Created a separate ResourceRateLimit model rather than re-purpose the existing RateLimit. I've chosen to keep them separate as they have such different source (API vs Header) and could be changed by GitHub independently.
Created a structure made up of MiscRateLimt/ ResourcesRateLimits/ ResourcesRateLimit using the Poco Json deserialization - fitting the API structure. Possibly better to perform custom deserialization (similar to ActivityPayload?) to return an array of a simple object (resource name, limit, remaining, reset). Note entirely sure of the names either.
I've included the depreciating "Rate" for completeness. Given that it will depreciate, then maybe better to simply not get anyone into bad habits. Would be removed anyway if I go for custom deserialization.
To utilize Poco Json deserialization I've created a reset (long) and resetAsDateTimeOffset. Better way to do? Again, if I go down custom deserialization then I can just use the reset as DateTimeOffset.

Feedback always welcomed.

Red-Folder · 2015-07-24T21:03:00Z

Further thoughts on point 1;
If I re-purpose the existing RateLimit model then any consumer code doesn't need to handle the (logically) same data twice

haacked · 2015-07-24T21:03:08Z

Octokit/Models/Response/MiscRateLimits.cs

+namespace Octokit
+{
+    [DebuggerDisplay("{DebuggerDisplay,nq}")]
+    public class MiscRateLimits


We try to avoid abbreviations. So I'd call this MiscellaneousRateLimit

Changed as suggested

haacked · 2015-07-24T21:11:47Z

If I re-purpose the existing RateLimit model then any consumer code doesn't need to handle the (logically) same data twice

Yeah, I don't think we're likely to change RateLimit in any way that would change them in one place, but not the other. As far as I can tell, they're the same type in the back-end. So I think re-using RateLimit and not introducing a new type is the way to go. 😄

Thanks for the contribution!

Red-Folder · 2015-07-26T21:53:14Z

@haacked AppVeyor isn't building, is this because the original ResourceRateLimit.cs comment is still open, or is it something wrong with the code?

Originally I though it was the code - that I needed to put some #if !NETFX_CORE blocks round attributes. But I've made changes to that an it doesn't seem to make any change.

shiftkey · 2015-07-26T22:16:45Z

@Red-Folder it looks like there's a merge conflict with this branch - AppVeyor should go green after that's been addressed.

shiftkey · 2015-07-26T23:01:38Z

Octokit/Octokit-Mono.csproj

@@ -396,7 +396,12 @@
    <Compile Include="Exceptions\TwoFactorAuthorizationException.cs" />
    <Compile Include="Http\HttpMessageHandlerFactory.cs" />
    <Compile Include="Models\Response\TeamMembership.cs" />
+<<<<<<< HEAD


Looks like these files need to be unified...

Looking into it now

Red-Folder · 2015-07-26T23:34:31Z

Ok, green now. Now I leave it alone and get some sleep.

Lessons learnt this evening:

Do a fetch & merge from upstream BEFORE pushing
How to use Meld
That I should double check everything is as I expect in the commit
Run the automated tests (even after I make the smallest possible change that in no possible way could break the solution)
I should have gone to bed about 2 hours ago. I'm too old to be up past midnight anymore

On a more serious note;

Appreciate the feedback & assistance given - please keep it coming.
I still need to run the integration tests (for some reason I cant make run yet - keep getting bad credentials)

shiftkey · 2015-07-26T23:35:57Z

@Red-Folder given this is a new feature (and the test is written) I'm happy to take it in as-is.

Don't stress too much about running the integration tests, it's certainly the toughest part of contributing (and something I need to revisit).

shiftkey · 2015-07-26T23:36:56Z

Octokit.Tests/Clients/MiscellaneousClientTests.cs

+                var result = await client.GetRateLimits();
+
+                // Test the high level object
+                Assert.NotNull(result);


While I love the explicitness here, I think letting this fall through to the Assert.Equals is totally fine to simplify the tests...

Just to clarify, is this the NotNull check? What about the other NotNull tests further down - lose them as well to make more readable?

I assumed the above was yes & yes

shiftkey · 2015-07-26T23:38:05Z

Octokit/Http/RateLimit.cs


 namespace Octokit
 {
 #if !NETFX_CORE
    [Serializable]
+    [DebuggerDisplay("{DebuggerDisplay,nq}")]


I think this can live outside the #if

Will do. I changed all this when I thought it was causing the appveyor problems (there was reasoning behind my thinking but I won't bore you)

Added GetRateLimits to MiscellaneousClient

haacked · 2015-07-27T21:01:05Z

Thanks for your contribution!

Red-Folder · 2015-08-02T15:16:50Z

Is it wrong that after 20+ years in IT, I'm seeing a 6 second vine from Phil Hacck as my most glowing accreditation?

Surely there must be a "thumbs up" accreditation section somewhere in Linkedin .... off to go look

haacked · 2015-08-03T16:36:15Z

Surely there must be a "thumbs up" accreditation section somewhere in Linkedin .... off to go look

LOL! It's a GitHub special accreditation. Much better than LinkedIn.

Added GetRateLimits to MiscellaneousClient

d88f180

haacked reviewed Jul 24, 2015
View reviewed changes

Mark Taylor added 2 commits July 26, 2015 22:03

Additional changes to support rate_limit API call

e092a10

Added changes to handle NETFX_CORE properly (I hope)

d5d264d

Resolved merge conflicts in project files

2314b62

shiftkey reviewed Jul 26, 2015
View reviewed changes

Fixes to project files after corrupting them with merge

0405718

shiftkey reviewed Jul 26, 2015
View reviewed changes

shiftkey changed the title ~~[WIP] Added GetRateLimits to MiscellaneousClient~~ Added GetRateLimits to MiscellaneousClient Jul 26, 2015

shiftkey reviewed Jul 26, 2015
View reviewed changes

Additional changes following review

4d9cc4f

haacked added a commit that referenced this pull request Jul 27, 2015

Merge pull request #848 from Red-Folder/master

a7bc09e

Added GetRateLimits to MiscellaneousClient

haacked merged commit a7bc09e into octokit:master Jul 27, 2015

Red-Folder mentioned this pull request Jul 29, 2015

Rate Information #811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added GetRateLimits to MiscellaneousClient #848

Added GetRateLimits to MiscellaneousClient #848

Red-Folder commented Jul 24, 2015

Red-Folder commented Jul 24, 2015

haacked Jul 24, 2015

Red-Folder Jul 26, 2015

haacked commented Jul 24, 2015

Red-Folder commented Jul 26, 2015

shiftkey commented Jul 26, 2015

shiftkey Jul 26, 2015

Red-Folder Jul 26, 2015

Red-Folder commented Jul 26, 2015

shiftkey commented Jul 26, 2015

shiftkey Jul 26, 2015

Red-Folder Jul 27, 2015

Red-Folder Jul 27, 2015

shiftkey Jul 26, 2015

Red-Folder Jul 27, 2015

haacked commented Jul 27, 2015

Red-Folder commented Aug 2, 2015

haacked commented Aug 3, 2015

Added GetRateLimits to MiscellaneousClient #848

Added GetRateLimits to MiscellaneousClient #848

Conversation

Red-Folder commented Jul 24, 2015

Red-Folder commented Jul 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haacked commented Jul 24, 2015

Red-Folder commented Jul 26, 2015

shiftkey commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Red-Folder commented Jul 26, 2015

shiftkey commented Jul 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haacked commented Jul 27, 2015

Red-Folder commented Aug 2, 2015

haacked commented Aug 3, 2015