-
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
Access to the last ApiInfo object #855
Changes from 1 commit
f8ee4f9
a6a6fdf
1acdd54
bc5a14a
6735b22
40f1e59
ba36524
5bd1f1d
b2c7e1c
1d695f1
07d6a8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
using Octokit.Tests.Integration; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
using Xunit; | ||
|
||
public class GitHubClientTests | ||
{ | ||
public class TheLastApiInfoProperty | ||
{ | ||
[IntegrationTest] | ||
public async Task CanRetrieveLastApiInfo() | ||
{ | ||
var github = Helper.GetAuthenticatedClient(); | ||
|
||
// Doesn't matter which API gets called | ||
await github.Miscellaneous.GetRateLimits(); | ||
|
||
var result = github.LastApiInfo; | ||
|
||
//Assert.True(result.Links.Count > 0); | ||
//Assert.True(result.AcceptedOauthScopes.Count > 0); | ||
//Assert.True(result.OauthScopes.Count > 0); | ||
//Assert.False(String.IsNullOrEmpty(result.Etag)); | ||
Assert.True(result.RateLimit.Limit > 0); | ||
Assert.True(result.RateLimit.Remaining > -1); | ||
Assert.NotNull(result.RateLimit.Reset); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,12 @@ public HttpClientAdapter(Func<HttpMessageHandler> getHandler) | |
_http = new HttpClient(new RedirectHandler { InnerHandler = getHandler() }); | ||
} | ||
|
||
/// <summary> | ||
/// Gets the latest API Info - this will be null if no API calls have been made | ||
/// </summary> | ||
/// <returns><seealso cref="ApiInfo"/> representing the information returned as part of an Api call</returns> | ||
public ApiInfo LastApiInfo { get; private set; } | ||
|
||
/// <summary> | ||
/// Sends the specified request and returns a response. | ||
/// </summary> | ||
|
@@ -45,7 +51,11 @@ public async Task<IResponse> Send(IRequest request, CancellationToken cancellati | |
// Make the request | ||
var responseMessage = await _http.SendAsync(requestMessage, HttpCompletionOption.ResponseContentRead, cancellationTokenForRequest) | ||
.ConfigureAwait(false); | ||
return await BuildResponse(responseMessage).ConfigureAwait(false); | ||
var response = await BuildResponse(responseMessage).ConfigureAwait(false); | ||
|
||
LastApiInfo = response.ApiInfo; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not have to be set here and IMO it's not a HTTP concern at all. All requests go trough There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, what about threading? Should there be a lock introduced here? |
||
|
||
return response; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
|
||
namespace Octokit | ||
{ | ||
/// <summary> | ||
/// Provides a property for the Last recorded API infomation | ||
/// </summary> | ||
public interface IApiInfo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds like an interface for the |
||
{ | ||
/// <summary> | ||
/// Gets the latest API Info - this will be null if no API calls have been made | ||
/// </summary> | ||
/// <returns><seealso cref="ApiInfo"/> representing the information returned as part of an Api call</returns> | ||
ApiInfo LastApiInfo { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ namespace Octokit.Internal | |
/// <remarks> | ||
/// Most folks won't ever need to swap this out. But if you're trying to run this on Windows Phone, you might. | ||
/// </remarks> | ||
public interface IHttpClient : IDisposable | ||
public interface IHttpClient : IDisposable, IApiInfo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does not feel right. API info is not a HTTP concern (see previous comment for a potention solution). |
||
{ | ||
/// <summary> | ||
/// Sends the specified request and returns a response. | ||
|
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.
@khellang I've added this for the thread safety. Thoughts welcome
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.
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.
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.
@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)
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'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.
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.
(Off topic)
Please tell me someone out there is photoshoping the below - replacing with @haacked and the Octocat (not saying which way round)
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 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.
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'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 theApiInfo
to reduce the sharing of mutable state. In fact, I thinkLastApiInfo
should be a methodGetLastApiInfo
and it should always return a copy of the lastApiInfo
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?
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.
SGTM 👍