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.
Master PR for Pull Request Reviews #1491
Conversation
As GitHub won't let us create empty PRs.
There was code left over from the now punted "conversation" view. Remove this code because by the time we get back to it, it will be out of date anyway.
And removed missing files.
…iews-code Removed unused InlineReviews code.
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
Enable navigation from diff view to editor
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.
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.
Ported from #1415. Moves the PR details changed files tree into its own view so that it can be shared by the PR reviews view.
Enable navigation from diff view to editor
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.
Factor out `EnableNavigateStatusBarMessage` method.
| <HintPath>..\..\packages\Newtonsoft.Json.10.0.3\lib\net45\Newtonsoft.Json.dll</HintPath> | ||
| <Private>True</Private> | ||
| </Reference> | ||
| <Reference Include="Microsoft.VisualStudio.Utilities, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL"> |
jcansdale
Apr 10, 2018
Collaborator
We have a duplicate Microsoft.VisualStudio.Utilities reference.
We have a duplicate Microsoft.VisualStudio.Utilities reference.
|
Should I be able to click on the |
Conflicts: src/GitHub.Exports/Models/UsageModel.cs
|
I haven't scrutinized every line, but I've been dogfooding it quite a bit and it has been working well! |
No, I misremembered. There was one about making the avatar clickable not the |
|
This PR LGTM I ran through these test scenarios and did some exploratory testing as well. |
|
LGTM! Left some comments/questions, but they aren't blockers. Reviewed this in experimental version! |
| public int Id { get; set; } | ||
| public string NodeId { get; set; } |
sguthals
Apr 13, 2018
Contributor
What is the difference between Id and NodeId?
What is the difference between Id and NodeId?
grokys
Apr 16, 2018
Author
Contributor
As you noticed later, NodeId is the GraphQL ID which is different from the "database" Id. I didn't add doc comments to these fields because this will all change as we move move stuff to GraphQL, but I probably should have.
As you noticed later, NodeId is the GraphQL ID which is different from the "database" Id. I didn't add doc comments to these fields because this will all change as we move move stuff to GraphQL, but I probably should have.
| public IReadOnlyCollection<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0]; | ||
| public IReadOnlyCollection<ICommentModel> Comments { get; set; } = new ICommentModel[0]; | ||
| public IReadOnlyList<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0]; | ||
| public IReadOnlyList<ICommentModel> Comments { get; set; } = new ICommentModel[0]; |
sguthals
Apr 13, 2018
Contributor
Why switch from IReadOnlyColletion to IReadOnlyList?
Why switch from IReadOnlyColletion to IReadOnlyList?
grokys
Apr 16, 2018
Author
Contributor
IReadOnlyCollection doesn't allow indexers (e.g. collection[0]) which is useful in unit tests. There was no reason to use collection over list so I moved over to that.
IReadOnlyCollection doesn't allow indexers (e.g. collection[0]) which is useful in unit tests. There was no reason to use collection over list so I moved over to that.
| @@ -69,8 +67,35 @@ public PullRequestDetailViewModelDesigner() | |||
| modelsDir.Files.Add(oldBranchModel); | |||
| gitHubDir.Directories.Add(modelsDir); | |||
|
|
|||
| changedFilesTree = new List<IPullRequestChangeNode>(); | |||
| changedFilesTree.Add(gitHubDir); | |||
| Reviews = new[] | |||
sguthals
Apr 13, 2018
Contributor
So this is just a model of the type of data that would be included in a real PR Review that you model here?
So this is just a model of the type of data that would be included in a real PR Review that you model here?
sguthals
Apr 13, 2018
Contributor
Since this is all in sample data. I get it now.
Since this is all in sample data. I get it now.
grokys
Apr 16, 2018
Author
Contributor
Yep, sample data for the designer.
Yep, sample data for the designer.
|
|
||
| namespace GitHub.SampleData | ||
| { | ||
| public class PullRequestReviewFileCommentViewModelDesigner : IPullRequestReviewFileCommentViewModel |
sguthals
Apr 13, 2018
Contributor
Why dont' we have sample data here?
Why dont' we have sample data here?
grokys
Apr 16, 2018
Author
Contributor
Because this class isn't used directly by the designer, it's used by other sample data view models, so they set the sample data. See https://github.com/github/VisualStudio/pull/1491/files/692beca0cb8b2139246cfa8ce4b71e9e88ba037e#diff-b62ccfc33b879b821bb668fd4cdbee75R37 for example.
Because this class isn't used directly by the designer, it's used by other sample data view models, so they set the sample data. See https://github.com/github/VisualStudio/pull/1491/files/692beca0cb8b2139246cfa8ce4b71e9e88ba037e#diff-b62ccfc33b879b821bb668fd4cdbee75R37 for example.
|
|
||
| // HACK: We need to wait here for the diff view to set itself up and move its cursor | ||
| // to the first changed line. There must be a better way of doing this. | ||
| await Task.Delay(1500); |
sguthals
Apr 13, 2018
Contributor
Should we make a new issue to address this later?
Should we make a new issue to address this later?
grokys
Apr 16, 2018
Author
Contributor
Yes, probably, we've had this hack in there since inline comments was first introduced.
Yes, probably, we've had this hack in there since inline comments was first introduced.
| statusBar.ShowMessage(message + ": " + e.Message); | ||
| } | ||
|
|
||
| void AddBufferTag( |
sguthals
Apr 13, 2018
Contributor
What is the buffer tag? Is it the "PR 1491" that is to the right of the file name on the right diff?
What is the buffer tag? Is it the "PR 1491" that is to the right of the file name on the right diff?
grokys
Apr 16, 2018
Author
Contributor
It's a way of attaching our own data to a text buffer describing what PR/commit/file a diff view represents. It adds an instance of PullRequestTextBufferInfo as a property of the text buffer. That class is documented (though probably not well enough) instead of this method to avoid duplicating documentation. Might be worth documenting both though if it's confusing.
It's a way of attaching our own data to a text buffer describing what PR/commit/file a diff view represents. It adds an instance of PullRequestTextBufferInfo as a property of the text buffer. That class is documented (though probably not well enough) instead of this method to avoid duplicating documentation. Might be worth documenting both though if it's confusing.
sguthals
Apr 16, 2018
Contributor
Maybe worth just putting:
It adds an instance of PullRequestTextBufferInfo as a property of the text buffer.
Maybe worth just putting:
It adds an instance of PullRequestTextBufferInfo as a property of the text buffer.
| } | ||
| catch (Exception) | ||
| { | ||
| // TODO: Show error. |
sguthals
Apr 13, 2018
Contributor
Does this need to be added before the merge?
Does this need to be added before the merge?
grokys
Apr 16, 2018
Author
Contributor
Ah, yes, I will address this together with the double-click exceptions in a separate PR.
Ah, yes, I will address this together with the double-click exceptions in a separate PR.
| /// <summary> | ||
| /// Gets the GraphQL ID for the review. | ||
| /// </summary> | ||
| string NodeId { get; set; } |
sguthals
Apr 13, 2018
Contributor
Aha! So Node Id is for GraphQL. Why is that different from the Id of the review though still?
Aha! So Node Id is for GraphQL. Why is that different from the Id of the review though still?
grokys
Apr 16, 2018
Author
Contributor
Well... That's probably quite a long explanation ;) The short answer is that the REST API exposed database IDs whereas GraphQL uses global relay IDs. At the moment we're using both APIs so for certain models we need both IDs in order to interoperate. This will change hopefully very soon as we move more stuff to GraphQL.
Well... That's probably quite a long explanation ;) The short answer is that the REST API exposed database IDs whereas GraphQL uses global relay IDs. At the moment we're using both APIs so for certain models we need both IDs in order to interoperate. This will change hopefully very soon as we move more stuff to GraphQL.
sguthals
Apr 16, 2018
Contributor
Oh got it - that makes sense for sure :) Thanks!
Oh got it - that makes sense for sure :) Thanks!
…or-discoverable Make Navigate to Editor more discoverable



This is the master PR for the Pull Request Reviews feature (spiked out over at #1415).