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

Cannot get comments via octokit #1495

Closed
jaredpar opened this issue Oct 24, 2016 · 9 comments
Closed

Cannot get comments via octokit #1495

jaredpar opened this issue Oct 24, 2016 · 9 comments

Comments

@jaredpar
Copy link

Consider the following code:

var client = new GitHubClient(new ProductHeaderValue("ReproProblem"));
var pr = await client.PullRequest.Get("dotnet", "roslyn", 14645);
Console.WriteLine(pr.Comments);
var comments = await client.PullRequest.Comment.GetAll("dotnet", "roslyn", pr.Number);
Console.WriteLine(comments.Count);

This is how I would expect to retrieve the comments for a specific PR. If you run the code though you will see it print out the following

4
0

Essentially the PR claims to have 4 comments (and if you look at the PR in question you will see it does). Yet the GetAll method is returning an empty collection.

Is there something I'm doing wrong here?

@shiftkey
Copy link
Member

shiftkey commented Oct 24, 2016

Nope, looks like the GitHub API is now using PullRequest.Comment to return Review Comments rather than comments on the PR itself. I'll ask around and see what's changed.

I'm not sure how long this is guaranteed to work, but here's my workaround:

var comments = await client.Issue.Comment.GetAllForIssue("dotnet", "roslyn", 14645);
Console.WriteLine(comments.Count);

@jaredpar
Copy link
Author

Thanks for the work around. That got me unblocked.

@pjc0247
Copy link
Contributor

pjc0247 commented Nov 9, 2016

@shiftkey But.. I think there's no API which can handle optional parameters in /repos/:owner/:repo/issues/comments.

Issue.Comment.GetAllForRepository
PullRequest.Comment.GetAllForRepository

Thanks

@shiftkey
Copy link
Member

shiftkey commented Nov 9, 2016

@pjc0247 I'm not clear what you were looking for in terms of options here - could you elaborate?

EDIT: ah yes, I see what's missing. I'll open an issue for this.

@ryangribble
Copy link
Contributor

Given that #1500 is opened, are we happy to close this one?

@shiftkey
Copy link
Member

@ryangribble I think this is a separate issue to #1500. If this endpoint is now serving up different data, we need to address this as a breaking change (introduce PullRequest.ReviewComment and obsolete PullRequest.Comment maybe?)

@ryangribble
Copy link
Contributor

OK so did you get confirmation that this had changed upstream?

I thought for as long as Ive been using octokit it was like this - Eg since PullRequests are also/actually Issues, the "comments" you see on the discussion area were always obtained from getting them from the Issue API, and the "comments" on the PullRequest API were always the "review" comments... even before all the changes to make code reviews a native/first class concept.

I just want to clarify with @jaredpar you said this is how you expect it to work, not necessarily that it used to work like this and has since changed, right?

I dont disagree that we could rename the PullRequest.Comment API cliient to PullRequest.ReviewComment but just trying to quantify whether this was an upstream change or just a legacy/confusing name scenario

@jaredpar
Copy link
Author

I just want to clarify with @jaredpar you said this is how you expect it to work, not necessarily that it used to work like this and has since changed, right?

Correct. The code sample I posted is just my expectation of how the API should work.

@ryangribble
Copy link
Contributor

OK, since this was just an expectation thing rather than any kind of breaking change, Im going to close this issue out. Ive also raised #1511 to tweak the naming of PullRequest.Comment to PullRequest.ReviewComment to make the naming clearer and better aligned with the API docs

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

No branches or pull requests

4 participants