Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
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)); |
StanleyGoldman
Feb 22, 2018
Contributor
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
query gets a single page or reviews and for each review a single page of comments.
GetPullRequestReviewComments will continue to query the comments per review.- The containing
for loop will continue this process for each page in this page of reviews
- The containing
while loop will continue this process to obtain all review pages
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
grokys
Mar 19, 2018
Author
Contributor
Yep, good point - added some comments, does that make it clearer?
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.
c649c25
to
0a20262
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
|
Looking good. A few comments but nothing serious. |
| @@ -366,6 +375,158 @@ IObservable<IReadOnlyList<IRemoteRepositoryModel>> GetOrganizationRepositories(s | |||
| }); | |||
| } | |||
|
|
|||
| #pragma warning disable CS0618 // DatabaseId is marked obsolete by GraphQL but we need it | |||
jcansdale
Mar 23, 2018
Collaborator
🚲 You could create a GetDatabaseId method surrounded by #pragma warning disable CS0618 to avoid turning off obsolete warnings for a large block.
GetDatabaseId method surrounded by #pragma warning disable CS0618 to avoid turning off obsolete warnings for a large block.
grokys
Mar 29, 2018
Author
Contributor
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.
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) |
jcansdale
Mar 23, 2018
Collaborator
I'm guessing this will show a build warning? Replace with catch { }. Trying to keep the build clean. 😉
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