Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Master PR for Pull Request Reviews #1491
Conversation
grokys
added
the
WIP
label
Feb 15, 2018
This was referenced Feb 15, 2018
This was referenced Mar 6, 2018
meaghanlewis
added this to To do
in 2.4.4
via automation
Mar 12, 2018
grokys
and others
added some commits
Mar 16, 2018
jcansdale
added some commits
Apr 10, 2018
| + <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"> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jcansdale
Apr 10, 2018
Contributor
Should I be able to click on the
|
Should I be able to click on the |
meaghanlewis
referenced this pull request
Apr 11, 2018
Closed
"This document is opened by another project" - when navigating to PR file #1583
jcansdale
and others
added some commits
Apr 11, 2018
jcansdale
reviewed
Apr 13, 2018
I haven't scrutinized every line, but I've been dogfooding it quite a bit and it has been working well!
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
jcansdale
Apr 13, 2018
Contributor
Should I be able to click on the
💬 on the reviewers section? I thought that was one of the PRs that got merged.😕 I don't seem to be able to in this version.
No, I misremembered. There was one about making the avatar clickable not the
No, I misremembered. There was one about making the avatar clickable not the |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
meaghanlewis
Apr 13, 2018
Contributor
This PR LGTM
I ran through these test scenarios and did some exploratory testing as well.
|
This PR LGTM I ran through these test scenarios and did some exploratory testing as well. |
meaghanlewis
added
the
verified
label
Apr 13, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment Hide comment
sguthals
reviewed
Apr 13, 2018
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; } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
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.
grokys
Apr 16, 2018
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.
| - 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]; |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
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.
grokys
Apr 16, 2018
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.
| @@ -69,8 +67,35 @@ public PullRequestDetailViewModelDesigner() | ||
| modelsDir.Files.Add(oldBranchModel); | ||
| gitHubDir.Directories.Add(modelsDir); | ||
| - changedFilesTree = new List<IPullRequestChangeNode>(); | ||
| - changedFilesTree.Add(gitHubDir); | ||
| + Reviews = new[] |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
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?
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?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
| + | ||
| +namespace GitHub.SampleData | ||
| +{ | ||
| + public class PullRequestReviewFileCommentViewModelDesigner : IPullRequestReviewFileCommentViewModel |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
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.
grokys
Apr 16, 2018
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.
| + | ||
| + // 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); |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
Contributor
Yes, probably, we've had this hack in there since inline comments was first introduced.
grokys
Apr 16, 2018
Contributor
Yes, probably, we've had this hack in there since inline comments was first introduced.
| + statusBar.ShowMessage(message + ": " + e.Message); | ||
| + } | ||
| + | ||
| + void AddBufferTag( |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
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?
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?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
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.
grokys
Apr 16, 2018
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.
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
sguthals
Apr 16, 2018
Contributor
Maybe worth just putting:
It adds an instance of PullRequestTextBufferInfo as a property of the text buffer.
sguthals
Apr 16, 2018
Contributor
Maybe worth just putting:
It adds an instance of PullRequestTextBufferInfo as a property of the text buffer.
| + } | ||
| + catch (Exception) | ||
| + { | ||
| + // TODO: Show error. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
Contributor
Ah, yes, I will address this together with the double-click exceptions in a separate PR.
grokys
Apr 16, 2018
Contributor
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; } |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
sguthals
Apr 13, 2018
Contributor
Aha! So Node Id is for GraphQL. Why is that different from the Id of the review though still?
sguthals
Apr 13, 2018
Contributor
Aha! So Node Id is for GraphQL. Why is that different from the Id of the review though still?
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Show comment
Hide comment
grokys
Apr 16, 2018
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.
grokys
Apr 16, 2018
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.



grokys commentedFeb 15, 2018
•
edited by jcansdale
Edited 34 times
-
jcansdale
edited Apr 16, 2018
-
jcansdale
edited Apr 16, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 10, 2018
-
jcansdale
edited Apr 9, 2018
-
jcansdale
edited Apr 9, 2018
-
jcansdale
edited Apr 9, 2018
-
jcansdale
edited Apr 9, 2018
-
grokys
edited Apr 5, 2018
-
grokys
edited Apr 5, 2018
-
grokys
edited Apr 5, 2018
-
grokys
edited Apr 4, 2018
-
jcansdale
edited Apr 4, 2018
-
grokys
edited Apr 4, 2018
-
grokys
edited Apr 4, 2018
-
grokys
edited Mar 30, 2018
-
jcansdale
edited Mar 28, 2018
-
jcansdale
edited Mar 28, 2018
-
grokys
edited Mar 27, 2018
-
jcansdale
edited Mar 23, 2018
-
grokys
edited Mar 23, 2018
-
grokys
edited Mar 21, 2018
-
grokys
edited Mar 20, 2018
-
grokys
edited Mar 19, 2018
-
grokys
edited Mar 6, 2018
-
grokys
edited Mar 6, 2018
-
grokys
edited Feb 21, 2018
-
grokys
edited Feb 21, 2018
-
grokys
edited Feb 15, 2018
This is the master PR for the Pull Request Reviews feature (spiked out over at #1415).