Use GraphQL in ModelService to read PR reviews/comments. #1501
Conversation
|
The |
|
|
||
| if (review.CommentPage.HasNextPage) | ||
| { | ||
| result.AddRange(await GetPullRequestReviewComments(review.Id, review.CommentPage.EndCursor)); |
There was a problem hiding this comment.
We should throw in some comments that explain this GraphQL a bit, because the pagination is nuanced.
Let me take a stab at it...
- The above
querygets a single page or reviews and for each review a single page of comments. GetPullRequestReviewCommentswill continue to query the comments per review.- The containing
forloop will continue this process for each page in this page of reviews - The containing
whileloop will continue this process to obtain all review pages
There was a problem hiding this comment.
Yep, good point - added some comments, does that make it clearer?
And associated factory/keychain classes.
This is integrated a bit hackily into `ModelService`; `ModelService` doesn't really mesh well with GraphQL but without a lot of refactoring this was the best way to get things up and running.
As it's ignored by default.
The cursor was not being used for the first page, or ever in fact because the capitalization was wrong.
...in UnitTests. Akavache uses Newtonsoft.Json 6.0.8 while we use 10.0.3. The earlier version needs to be redirected to the later version for tests to work.
grokys
force-pushed
the
feature/graphql-modelservice-pr-reviews
branch
from
c649c25 to
0a20262
Compare
No, that branch can be removed now I think - the stuff in there that were needed by GHfVS has been added to master (strong name signing). |
…rvice-pr-reviews Conflicts: src/GitHub.App/packages.config
| }); | ||
| } | ||
|
|
||
| #pragma warning disable CS0618 // DatabaseId is marked obsolete by GraphQL but we need it |
There was a problem hiding this comment.
🚲 You could create a GetDatabaseId method surrounded by #pragma warning disable CS0618 to avoid turning off obsolete warnings for a large block.
There was a problem hiding this comment.
That won't work here as these expressions are converted at runtime to GraphQL queries, and the expression rewriter can't introspect such a method, so building the query will fail.
| } | ||
| return data; | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
I'm guessing this will show a build warning? Replace with catch { }. Trying to keep the build clean. 😉
Ported from #1415.
The REST API filters out pending reviews when reading PR reviews/review comments, making it unusable for the PR reviews feature. This PR updates
ModelServiceto take a GraphQLIConnectionand uses that to read PR review comments from the GraphQL API, and in addition reads the related PR reviews too.This is integrated a bit hackily into
ModelService;ModelServicedoesn't really mesh well with GraphQL, and our existing models are a bit of a strange fit for the (more logical) way that GraphQL uses nested models to represent relationships. However, without a lot of refactoring this was the best way to get things up and running, and more importantly I don't really have a good feel yet as to how we should structure things with GraphQL. As we hopefully move more things to GraphQL in the coming months we should start to get a feel for that.(As a side note, take a look at how much code is required to do nested paging in GraphQL - I think making this work by default in Ocotokit.GraphQL will be a big deal)
Part of #1491