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 for Pull Request Models. #1712
Conversation
Now using a data model based on GraphQL (or at least a hypothetical future version of the GraphQL API based on conversations with that team).
Should really be using `ActorAvatarView` here but our mess of assemblies means that it'd have to be moved. Fix later.
If the basic logic doesn't work, likely the data is bad.
...`PullRequestUserReviewsViewModelTests`. The view now loads instantly :D
Mainly merging #1701 Conflicts: src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs src/GitHub.InlineReviews/SampleData/CommentThreadViewModelDesigner.cs src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs src/GitHub.InlineReviews/Services/PullRequestSession.cs src/GitHub.InlineReviews/Services/PullRequestSessionService.cs src/GitHub.InlineReviews/ViewModels/CommentThreadViewModel.cs src/GitHub.InlineReviews/ViewModels/CommentViewModel.cs src/GitHub.InlineReviews/ViewModels/ICommentThreadViewModel.cs test/GitHub.InlineReviews.UnitTests/ViewModels/PullRequestReviewCommentViewModelTests.cs
Because adding e.g. a PR review comment affects both the `PullRequestReviewModel` _and_ the `PullRequestDetailModel`, re-read the PR when a comment mutation is executed.
The placeholder should be in `Editing` state when it is shown.
| @@ -10,6 +10,11 @@ namespace GitHub.InlineReviews.UnitTests.ViewModels | |||
| { | |||
| public class NewInlineCommentThreadViewModelTests | |||
| { | |||
| public NewInlineCommentThreadViewModelTests() | |||
| { | |||
| Splat.ModeDetector.Current.SetInUnitTestRunner(true); | |||
jcansdale
Jun 12, 2018
Collaborator
We should probably move this into an assembly wide setup for the whole test assembly. This is possible with NUnit in a way it wasn't with xUnit.
For example we could move it into AssemblyInfo.cs like this:
Otherwise it's too easy to forget and end up with hanging or not depending on the order they're executed. 😨
We should probably move this into an assembly wide setup for the whole test assembly. This is possible with NUnit in a way it wasn't with xUnit.
For example we could move it into AssemblyInfo.cs like this:
Otherwise it's too easy to forget and end up with hanging or not depending on the order they're executed.
jcansdale
Jun 12, 2018
Collaborator
Alternatively we could do this as part of #1721, which I think would make more sense. 😉
Alternatively we could do this as part of #1721, which I think would make more sense.
|
I like how you've cleaned up the models. :-) Just a few questions, no blockers. |
| @@ -105,6 +105,7 @@ public void Dispose() | |||
| { string.Empty, new PullRequestDirectoryNode(string.Empty) } | |||
| }; | |||
|
|
|||
| await Task.Delay(0); | |||
jcansdale
Jun 12, 2018
Collaborator
What is the Task.Delay(0) for? Would this behave the same with Task.Yield()?
What is the Task.Delay(0) for? Would this behave the same with Task.Yield()?
grokys
Jun 13, 2018
Author
Contributor
Oops, that was left in from when I commented out the whole of the method. Putting a Task.Delay(0) in was my way of preventing the "method doesn't use await" error. Will remove.
Oops, that was left in from when I commented out the whole of the method. Putting a Task.Delay(0) in was my way of preventing the "method doesn't use await" error. Will remove.
| if (readPullRequest == null) | ||
| { | ||
| readPullRequest = new Query() | ||
| .Repository(Var(nameof(owner)), Var(nameof(name))) |
jcansdale
Jun 12, 2018
Collaborator
I'm curious why you're caching the compiled query here but not other queries? I've found an easy way to automatically handle the named arguments and caching if this is something users of GraphQL should be doing.
I'm curious why you're caching the compiled query here but not other queries? I've found an easy way to automatically handle the named arguments and caching if this is something users of GraphQL should be doing.
grokys
Jun 13, 2018
Author
Contributor
Basically just because this was written from scratch and the other queries were just modified. We should probably be doing the same with the other queries, not sure if that should be in this PR or another?
Basically just because this was written from scratch and the other queries were just modified. We should probably be doing the same with the other queries, not sure if that should be in this PR or another?
jcansdale
Jun 13, 2018
Collaborator
Can I show you my GraphQL prototype before we do the the same with the other queries? I'd like to find out if you think it's a good idea in principle before developing it any further. So maybe push this to another PR.
Can I show you my GraphQL prototype before we do the the same with the other queries? I'd like to find out if you think it's a good idea in principle before developing it any further. So maybe push this to another PR.
| @@ -15,9 +16,9 @@ | |||
|
|
|||
| <d:DesignData.DataContext> | |||
| <vm:PullRequestReviewSummaryViewModel Id="1" State="Pending" FileCommentCount="2"> | |||
| <vm:PullRequestReviewSummaryViewModel.User> | |||
| <!--<vm:PullRequestReviewSummaryViewModel.User> | |||
jcansdale
Jun 12, 2018
Collaborator
Is this commented out for a reason?
Is this commented out for a reason?
grokys
Jun 13, 2018
Author
Contributor
Fixed.
Fixed.
|
There seems to be an issue when attempting to leave comments on some files.
Above I'm attempting to leave a comment on the following line of this PR: |
|
@jcansdale are you sure this was caused by this PR? This doesn't look like anything that this PR should have changed. Can you leave a comment in the same position on master? |
|
@grokys @jcansdale I just tried to leave a comment in the same position on master and I do see this same error happening. |
|
Sorry about the This PR looks good. |
Conflicts: src/GitHub.Exports.Reactive/Services/IPullRequestSession.cs src/GitHub.InlineReviews/Services/IPullRequestSessionService.cs src/GitHub.InlineReviews/Services/PullRequestSession.cs src/GitHub.InlineReviews/Services/PullRequestSessionService.cs src/GitHub.InlineReviews/ViewModels/InlineCommentThreadViewModel.cs test/GitHub.InlineReviews.UnitTests/Services/PullRequestSessionTests.cs test/GitHub.InlineReviews.UnitTests/ViewModels/InlineCommentThreadViewModelTests.cs
Description
Previously we were using a mix of REST and GraphQL for Pull Requests and Pull Request Reviews, and our data model didn't fit either very well. This PR:
PullRequestDetailModelwhich is modeled after what we're hoping will be the future shape of the GraphQL pull request/reviews APIModelServiceand intoPullRequestSession- this is now the only place to get hold of a PR details model. Doing this means that all parts of the extension always agree about the state of a pull request, and prevents some unnecessary API reads that we were carrying out before - our UI should feel a lot snappier now!PullRequestDetailModelusing GraphQL. Because the GraphQL API for reviews and comments isn't complete yet, after reading this we massage the model into what should be the final shape after reading (seeBuildPullRequestThreadsThe old
PullRequestModelis still there (although in a cut-down form) and still read using REST. It is now used just for the PR list. This will be moved to GraphQL in a later PR.Testing
Basically, everything around pull requests/reviews/comments should work as before!
TODO
Mutationobject for auto-paging and failing