Conversation
JanKrivanek
left a comment
JanKrivanek
left a comment
There was a problem hiding this comment.
Overall looks good to go - I have just couple minor comments for considering.
A oneliner description of the PR intent would be nice as well ;-)
| finishedProjects++; | ||
| UpdateFooter(); | ||
|
|
||
| if (restoringProjects > 0) |
There was a problem hiding this comment.
Sorry I'm not very familiar with the the build events sequencing - is this the proper place? Or would the TargetFinished (for Restore target) be actually a better place?
| TerminalBufferLine? line = null; // TerminalBuffer.WriteNewLineAfterMidpoint($"{e.ProjectFile} is blocked by the MSBuild task."); | ||
| if (line is not null) |
There was a problem hiding this comment.
Should the commented code be uncommented, or should all this be actually completely removed?
| if (!TerminalBuffer.IsRestoring) | ||
| { | ||
| if (blockedProjects.TryGetValue(e.ProjectFile, out int lineId)) | ||
| if (e.TaskName.Equals("MSBuild")) | ||
| { | ||
| TerminalBuffer.DeleteLine(lineId); | ||
| if (projects.TryGetValue(e.BuildEventContext!.ProjectInstanceId, out ProjectNode? node)) | ||
| if (blockedProjects.TryGetValue(e.ProjectFile, out int lineId)) |
There was a problem hiding this comment.
nit: This looks like the condition should be just combined to get more compact and less indented code
| return; | ||
| } | ||
| // Get project id | ||
| int id = e.BuildEventContext!.ProjectInstanceId; |
There was a problem hiding this comment.
I know this is just copied, but I'd be more defensive here
int id = (e.?BuildEventContext ?? BuildEventContext.Invalid).ProjectInstanceId;
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
I didn't finish the full review because I tried this out and it doesn't look like it's working:
Note that RestoreTask is running but it's not in "restore mode".
| UpdateFooter(); | ||
| ShouldRerender = true, | ||
| }; | ||
| projects[id] = node; |
There was a problem hiding this comment.
Why allocate new over mutating the existing one in the reentrance case?
| projects[id] = node; | ||
| UpdateFooter(); | ||
|
|
||
| if (e.TargetNames?.Contains("Restore") == true) |
There was a problem hiding this comment.
I think it's worth it to explicitly specify OrdinalIgnoreCase here even if netfx makes us do it the ugly way
| if (e.TargetNames?.Contains("Restore") == true) | |
| if (e.TargetNames?.IndexOf("Restore", StringComparison.OrdinalIgnoreCase) > 0) |
| if (e.TargetNames?.Contains("Restore") == true) | ||
| { | ||
| TerminalBuffer.IsRestoring = true; | ||
| restoringProjects++; |
There was a problem hiding this comment.
I don't understand this counter--I expected something more like "remember the ID of the first project that starts Restore and be done when that project instance completes". Does that not work in some circumstance?
| $"MSBuild - Restore in progress - {FinishedProjects} finished projects" : | ||
| $"MSBuild - Build in progress - {FinishedProjects} finished projects"; |
There was a problem hiding this comment.
Future item: I'd like the entrypoint project listed here, so the text becomes something like Restoring foo.csproj or Building bar.sln.
3 participants
Fixes #
Context
Changes Made
Testing
Notes