Access to the last ApiInfo object #855

Red-Folder · 2015-07-31T09:43:44Z

As discussed in issue #504

Allows the user to get the contents of the last ApiInfo (so last Api call). Provided as a property of GitHubClient which calls a property of Connection then a property of HttpClientAdapter.

The most obvious use for it is to check the current rate limit without making a separate API call.

Two concerns for me:

Can't put a test in for the HttpClientAdapter. The underlying HttpClient is concrete and the send method is not virtual so can be substituted. Could abstract the HttpClient (interface and wrapper) - this would help to put more testing into for the HttpClientAdapter (currently quite light).
Must be a better design than having to implement a property through every layer

Thanks

khellang · 2015-07-31T09:49:57Z

Octokit/Http/IApiInfo.cs

+    /// <summary>
+    /// Provides a property for the Last recorded API infomation
+    /// </summary>
+    public interface IApiInfo


This sounds like an interface for the ApiInfo itself. What about IApiInfoProvider, it seems to be more in line with the comments as well?

khellang · 2015-07-31T10:24:26Z

@Red-Folder I sent you a PR at Red-Folder#1, I still haven't addressed the threading issue, though.

Red-Folder · 2015-07-31T13:19:44Z

@khellang Great stuff. I'll pull in over the weekend

Red-Folder · 2015-07-31T13:23:06Z

Can anyone recommend a good Api call to use for the integration test?

I'm current using the Rate Limit call - but this doesn't include links, oauth scope, accepted oauth scope, & etag,in the ApiInfo. I'd like an Api call which will populate all of these for testing

shiftkey · 2015-08-02T00:40:12Z

@Red-Folder I'd suggest a couple of calls:

ETag can be found by creating a new repository - that should be easy enough
Links are a bit trickier (because most of our existing APIs are going to follow them as part of pagination). You might be able to check for the last value after the pagination is done - so something like "list your repos" might work for you.
X-OAuth-Scopes and X-Accepted-OAuth-Scopes require the caller to be configured with token authentication - that depends on the OCTOKIT_OAUTHTOKEN environment variable being set, but the caller might not have this setup.

API Info Proposal

Red-Folder · 2015-08-02T15:59:08Z

Octokit/Http/Connection.cs

+        }
+        private ApiInfo _lastApiInfo;
+        private readonly object LastApiInfoLocker = new object();
+


@khellang I've added this for the thread safety. Thoughts welcome

Hmm, do we actually need locks here? If someone is reading the value just as another thread is writing the value, it sort of doesn't matter who wins. After all, the read value will only be slightly outdated.

In other words, I don't see any problems with race conditions in this case as this isn't data that's timing sensitive. I don't think torn reads are a problem because these are 32 bit references.

This value is going to be set on every request, I'd rather avoid a lock here unless it's absolutely needed.

@khellang @haacked I'm easy either way - with locking/ without

What's the preferred method of resolving coding decisions? A well argued debate between respectful & collaborate peers? Rocks/ Paper/ Scissors? A fight to the death?

(is it wrong to hope for the last one)

I've been taking the Benevolent Dictator role. 😛

I'd like to hear from @khellang if there are specific concerns with removing the locks. If not, let's remove them.

(Off topic)

Please tell me someone out there is photoshoping the below - replacing with @haacked and the Octocat (not saying which way round)

I think it's fine to remove the locks, it's probably a bit too much. I just brought it up because there's now some shared, mutable state going on.

👍

Let's remove the locks, but make sure we comment it and explain why so if someone else comes along, they understand why there's shared mutable state.

Also, one idea we should do is to set LastApiInfo to a copy of the ApiInfo to reduce the sharing of mutable state. In fact, I think LastApiInfo should be a method GetLastApiInfo and it should always return a copy of the last ApiInfo

That way, Octokit is in control of the lifetime of that object and a consumer can't inadvertently keep the object around longer than expected. Thoughts?

Red-Folder · 2015-08-02T17:20:37Z

@shiftkey Added further integration tests now - seems to cover all bases if a little repetitive. I've added a new test attribute for Personal Access Token - the OAuth/ AcceptedOAuth test uses that attribute.

Red-Folder · 2015-08-10T23:06:27Z

Hi guys, any further feedback on this one?

I'm in danger of losing my dev environment in the next few days so would like to get this PR boxed off.

Thanks

Red-Folder · 2015-08-16T20:51:57Z

Octokit/Http/ApiInfo.cs

+            // Seem to have to do this to pass a whole bunch of tests (for example Octokit.Tests.Clients.EventsClientTests.DeserializesCommitCommentEventCorrectly)
+            // I believe this has something to do with the Mocking framework.
+            if (this == null || this.Links == null || this.OauthScopes == null || this.RateLimit == null || this.Etag == null)
+                return null;


Extra points for explaining why I needed to add this null check. For some reason bundles of tests fail without it (comment and see what happens). I think it is something to do with how the mocking framework is handling the object - but can't understand why it touches it.

Red-Folder · 2015-08-16T20:53:46Z

Could someone have a look over my cloning logic. Seems to work - but I could do with a fresh set of eyes on it.

Note that I'm having to do new String(originalString.ToCharArray()) as nothing else (string.Copy for example) seems to work with portable version.

haacked · 2015-08-25T20:11:48Z

Octokit/Http/ApiInfo.cs

+        {
+            // Seem to have to do this to pass a whole bunch of tests (for example Octokit.Tests.Clients.EventsClientTests.DeserializesCommitCommentEventCorrectly)
+            // I believe this has something to do with the Mocking framework.
+            if (this == null || this.Links == null || this.OauthScopes == null || this.RateLimit == null || this.Etag == null)


When would this be null though?

Fair point.

I really not sure why I need this line at all - but something blew up within the mocking framework so I added everything and kitchen sink.

Maybe went a tad overboard ;)

haacked · 2015-08-25T20:19:57Z

Looks good. The title suggests this is a WIP. Is that still the case or is this good to merge?

Red-Folder · 2015-08-26T08:12:34Z

@haacked Sorry Phil, I'd not been chasing this. I'd lost my dev environment & between jobs (must find new job before wife makes me paint more fences).

Dev environment back up and running. Will make the two minor tweaks you mention above then should be good to merge. Probably today or tomorrow

haacked · 2015-08-26T21:12:10Z

@Red-Folder No problem! Thanks for continuing to work on this and good luck finding a new job!

Red-Folder · 2015-08-28T14:30:51Z

Pops head above the parapet ... "Looks good to go"

(Prepares to duck back down quickly)

Access to the last ApiInfo object

haacked · 2015-08-28T17:50:42Z

Thanks!

Red-Folder · 2015-08-28T18:00:53Z

Ah ... excellent. A second thumbs up to add to the collection.

I've added a whole section on my bio page for them ;)

http://www.red-folder.com/Home/MyBio (then scroll down). One day I'll setup the anchors, make the site size correctly, finish it, etc, etc, blah, blah

WIP checkin for getting last ApiInfo object

f8ee4f9

khellang reviewed Jul 31, 2015
View reviewed changes

khellang added 3 commits July 31, 2015 12:08

Renamed IApiInfo -> IApiInfoProvider

a6a6fdf

Removed IApiInfoProvider from IHttpClient

1acdd54

Fixed failing tests

bc5a14a

Red-Folder and others added 3 commits August 2, 2015 16:26

Merge pull request #1 from khellang/api-info

6735b22

API Info Proposal

Fixed merge conflicts

40f1e59

Added locking for thread saftey to LastApiInfo

ba36524

Red-Folder reviewed Aug 2, 2015
View reviewed changes

Added further integration tests for LastApiInfo

5bd1f1d

Mark Taylor added 2 commits August 16, 2015 21:27

Chnage LastApiInfo to GetLastApiInfo with cloned version

b2c7e1c

Merge remote-tracking branch 'upstream/master'

1d695f1

Red-Folder reviewed Aug 16, 2015
View reviewed changes

haacked reviewed Aug 25, 2015
View reviewed changes

Minor changes based on feedback

07d6a8a

Red-Folder changed the title ~~[WIP] Access to the last ApiInfo object~~ Access to the last ApiInfo object Aug 28, 2015

haacked added a commit that referenced this pull request Aug 28, 2015

Merge pull request #855 from Red-Folder/master

5a117fe

Access to the last ApiInfo object

haacked merged commit 5a117fe into octokit:master Aug 28, 2015

shiftkey mentioned this pull request Mar 11, 2016

Documentation for getting API information from GitHubClient. #1142

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access to the last ApiInfo object #855

Access to the last ApiInfo object #855

Red-Folder commented Jul 31, 2015

khellang Jul 31, 2015

khellang commented Jul 31, 2015

Red-Folder commented Jul 31, 2015

Red-Folder commented Jul 31, 2015

shiftkey commented Aug 2, 2015

Red-Folder Aug 2, 2015

haacked Aug 10, 2015

Red-Folder Aug 11, 2015

haacked Aug 11, 2015

Red-Folder Aug 11, 2015

khellang Aug 11, 2015

haacked Aug 11, 2015

khellang Aug 11, 2015

Red-Folder commented Aug 2, 2015

Red-Folder commented Aug 10, 2015

Red-Folder Aug 16, 2015

Red-Folder commented Aug 16, 2015

haacked Aug 25, 2015

Red-Folder Aug 26, 2015

haacked commented Aug 25, 2015

Red-Folder commented Aug 26, 2015

haacked commented Aug 26, 2015

Red-Folder commented Aug 28, 2015

haacked commented Aug 28, 2015

Red-Folder commented Aug 28, 2015

Access to the last ApiInfo object #855

Access to the last ApiInfo object #855

Conversation

Red-Folder commented Jul 31, 2015

Choose a reason for hiding this comment

khellang commented Jul 31, 2015

Red-Folder commented Jul 31, 2015

Red-Folder commented Jul 31, 2015

shiftkey commented Aug 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Red-Folder commented Aug 2, 2015

Red-Folder commented Aug 10, 2015

Choose a reason for hiding this comment

Red-Folder commented Aug 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haacked commented Aug 25, 2015

Red-Folder commented Aug 26, 2015

haacked commented Aug 26, 2015

Red-Folder commented Aug 28, 2015

haacked commented Aug 28, 2015

Red-Folder commented Aug 28, 2015