-
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
Implemented GetLatestRelease API. #975
Conversation
/// <param name="owner">The owner of the repository</param> | ||
/// <param name="name">The name of the repository</param> | ||
/// <returns></returns> | ||
public static Uri LatestReleases(string owner, string name) |
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.
💄 perhaps make this singular i.e. LatestRelease
Looking good so far @chenjiaming93! A couple of things I'd like to verify as part of integration tests:
|
I have added tests. This one is similar to GetRelease API. Just replaced release id with "latest". In the tests for getrelease we are only checking if we can receive the get request at that endpoint and throw proper exceptions when missing arguments. If there is no latest release, are we expected to get a Null JSON or we a 500 error? |
@chenjiaming93 we have some existing tests which use the real GitHub API - let's add a couple of tests for this endpoint there: https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Clients/ReleasesClientTests.cs A couple of tests I think we can do to complete this feature:
I'm honestly not sure - let's find out by writing the test! I think the end result should be a custom exception that we throw to the user. There's a detailed example here for how we handle when creating a repository fails - I think you'll have to do something similar and |
@shiftkey I see. I will add integration tests soon. |
Any update on this issue? |
@khalidabuhakmeh I am busy with my final exams this week. I will add integration tests this weekend. |
Happy New Year! @chenjiaming93 were you still planning to add integration tests? |
@haacked I'm just back to school. Will start working tmr |
I've moved the integration tests out to #1086 rather than hold this up any longer. Thanks for the contribution @chenjiaming93! |
Implemented GetLatestRelease API.
trying to fix
#966