Add progress abstraction for installation library #51463
Conversation
There was a problem hiding this comment.
This is a great improvement, thank you! I really like how you abstracted this.
I wonder if we should expand dotnet install root to print the full path instead of a relative path, but that's not really needed for this PR. It might look awkward if the dotnet install root is several directories above the user's working directory.
| MaxValue = maxValue | ||
| }; | ||
| _tasks.Add(task); | ||
| Spectre.Console.AnsiConsole.WriteLine(description + "..."); |
There was a problem hiding this comment.
nit: Should we add ... to the description every time? And should we abstract spectre.console out of the class? I don't think we need to do either for now.
|
Another remark before merging: I would stilll recommend working in The review comments here (#50988) won't apply to the dnup branch which might make it harder to see what we've fixed. If we want to work out of the release branch, which it sounds like you and @/marcpopmsft agree we should, then we should close or redo that PR once the feedback is addressed in the release branch. I have addressed a majority of the feedback in my working PR so it's less of an issue. However, another issue with targeting release/dnup is that the SDK checks run below, and we'd need to modify the github rules and sdk pipelines to exclude the release/dnup branch. When we get to the point where release/dnup modifies SDK code though, I think it makes sense to run the SDK checks there. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
2 participants
Installation library no longer uses Spectre.Console directly but now goes through an
IProgressTargetinterface. Updated the--nno-progressoption to use a different implementation of this interface, rather than passing the option as a parameter down to the library.Currently, the output looks like this (without progress updates and with them):