Skip to content
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

Implement Issue Timeline preview API #1435

Merged
merged 22 commits into from
Aug 7, 2016

Conversation

hnrkndrssn
Copy link
Contributor

@hnrkndrssn hnrkndrssn commented Aug 3, 2016

Adding the Issue Timeline API client as it is now in preview (and we have a need for it for an internal project).

  • Timeline API client
  • Observable Timeline API client
  • XML Doc comments
  • Unit tests
  • Observable unit tests
  • Integration tests
  • Observable integration tests
  • Repository Id overloads
  • Repository Id overload tests
  • Paging support
  • Paging support tests

Fixes #1436

@hnrkndrssn hnrkndrssn changed the title [WIP] Implement Issue Timeline preview API Implement Issue Timeline preview API Aug 6, 2016
@hnrkndrssn
Copy link
Contributor Author

All tests are now passing 🎉 so this is ready for someone to cast their 👀 over 😁

@ryangribble
Copy link
Contributor

This is all looking rather 💎 to me! Ive run the integration tests too 👍

One thing I noticed was a quick count of the "events" listed in the timeline API doc (21) is a couple more than values declared in the EventInfoState enum (19), so is there possibly another couple of values that need to be added to this enum in addition to the cross referenced one you added?

/// A client for GitHub's Issue Timeline API.
/// </summary>
/// <remarks>
/// See the <a href="http://developer.github.com/v3/issues/timeline/">Issue Timeline API documentation</a> for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick but the doc comments added in this PR have a mix of http and https on the URLs... 😉

Copy link
Contributor Author

@hnrkndrssn hnrkndrssn Aug 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very odd as I'm pretty sure I copied those from the API documentation...I'll fix it and check the docs if there might something pointing to just http.

Ah, the original comment I copied from (in IIssuesEventsClient) has http instead of https...that'll teach me for blindly copy-pasting 😄

@alexander-efremov
Copy link
Contributor

alexander-efremov commented Aug 6, 2016

@alfhenrik since version 0.21.1 every "owner/name" method has overload that use repository id instead of owner/name pair. Could you add it in your PR?

@ryangribble not sure, but I think that now every new clients have to have repostoryId overload for each owner/name method, aren't they?

@hnrkndrssn
Copy link
Contributor Author

Thanks @dampir, I'll add in the necessary overloads and tests

@ryangribble
Copy link
Contributor

@ryangribble not sure, but I think that now every new clients have to have repostoryId overload for each owner/name method, aren't they?

Good pickup, I actualy realised this myself overnight (the things you think about while going to sleep, lol!)

Yes we should be adding repositoryId overload of any method that takes owner/name (as long as it actually works of course!).

I also realised, we should be adding paging support to any GetAllxxx() type methods (ie an overload taking ApiOptions class, with the default method passing ApiOptions.None through to it). Again, assuming paging is actually supported on that end point of course.

@hnrkndrssn
Copy link
Contributor Author

hnrkndrssn commented Aug 6, 2016

I also realised, we should be adding paging support to any GetAllxxx() type methods (ie an overload taking ApiOptions class, with the default method passing ApiOptions.None through to it). Again, assuming paging is actually supported on that end point of course.

Looking at the API docs, it doesn't look like the endpoint supports paging, but I'll do some Fiddlering to see if it might actually support it, just being undocumented.

Well, Fiddlering confirms that the endpoint does support paging, I'll add that in as well

@hnrkndrssn hnrkndrssn changed the title Implement Issue Timeline preview API [WIP] Implement Issue Timeline preview API Aug 6, 2016
@hnrkndrssn hnrkndrssn changed the title [WIP] Implement Issue Timeline preview API Implement Issue Timeline preview API Aug 7, 2016
@hnrkndrssn
Copy link
Contributor Author

OK, this should now be good to go

@ryangribble
Copy link
Contributor

Nice work! Everything looking good, ill just pull down your branch and run through the integration tests 👍

@@ -341,6 +341,16 @@ public static Uri IssueReactions(int repositoryId, int number)
return "repositories/{0}/issues/{1}/reactions".FormatUri(repositoryId, number);
}

public static Uri IssueTimeline(string owner, string repo, int number)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XmlDoc comment on this function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ryangribble
Copy link
Contributor

I just noticed missing XmlDoc comments on the 2 new functions added to ApiUrls.cs helper class which we probably should add, since every other function in that class has doc tags currently...

Once that's done I'm good to merge 😀

@hnrkndrssn
Copy link
Contributor Author

Pretty sure that TravisCI failure isn't because of me...looks like that random build failure we've seen before...

@ryangribble
Copy link
Contributor

LGTM

@ryangribble ryangribble merged commit 23d9310 into octokit:master Aug 7, 2016
@hnrkndrssn hnrkndrssn deleted the enh-timelineclient branch August 7, 2016 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants