Workload commands: JSON only output adjustments

[MiYanni]

Fixes: #22887

Note

This PR is a breaking change.
I do plan to also work on this issue which would be good to lump together in the same breaking change doc.

Summary

There were a couple of primary changes:

• Removed the Start/End boundary lines from the JSON only workload commands
• Only have the JSON being written (no logging messages) for the JSON only workload commands

Commands adjusted:

• dotnet workload list --machine-readable
• dotnet workload install --print-download-link-only
• dotnet workload update --print-download-link-only
• dotnet workload update --print-rollback

Details

The first change (boundary lines) was very simple and just involved removing a couple of WriteLine outputs for each command. Then, adjusting tests accordingly.

The second change was a bit more involved. We use a Reporter to write out to the console. Think of it as just an output stream. Right now, these commands are designed to:

• Pass an instance of Reporter down through the workload command hierarchy.
• If one is not provided, the default Reporter.Output will be used.
• The Reporter property can only be set via this constructor hierarchy.

For the commands in question, we need to:

• Use the configured Reporter to write the JSON appropriately
• Every other usage of Reporter beyond writing the JSON is replaced with NullReporter.Instance

To do this was a bit tricky. I had to add the ability to pass a Reporter to several methods that utilized it. But there was something else that also needed adjusted. The PackageDownloader is also created deep in the hierarchy using the Reporter from the constructor. Since I actually want to utilize the Reporter being passed in to write the JSON, I can't just force the Reporter property to be NullReporter.

So, I ended up creating a new instance of NuGetPackageDownloader for this situation. It also allowed me to use a NullLogger which applies to the NuGet messages. For unit testing, I had to add IsPackageDownloaderProvided to indicate that a PackageDownloader was passed through the constructor hierarchy. Then, I use that PackageDownloader instead of creating a new one.

[ghost]

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

[Forgind]

Can you explain why you had to pass in the null reporter as opposed to just removing more instances of Reporter.WriteLine or equivalent? I'm thinking that if we decide the same NuGetPackageDownloader should print out something, we'd have to undo this change and fix it another way, then add on that change. At minimum, perhaps we could use a boolean to say "write stuff" versus not?

[MiYanni]

@Forgind

Can you explain why you had to pass in the null reporter as opposed to just removing more instances of Reporter.WriteLine or equivalent?

The lines written by those other classes are used by all the other commands that don't feature the flags that print out JSON. So, the commands (list, install, update) that don't have the flags (--machine-readable, --print-download-link-only, --print-rollback) respectively will still use the normal Reporter and print these lines as expected. The purpose of the PR is to make these flags only allow JSON to be printed so tooling can pipe the output if these commands directly into a JSON parser.

At minimum, perhaps we could use a boolean to say "write stuff" versus not?

The main issue is just a result of the class hierarchy and construction. Also, the combination of the "good path" constructor (path the code normally takes when running) with the "unit test" constructor makes it a bit convoluted in how to solve this without breaking tests. Most designs have these as 2 separate constructors.

Adding a Boolean doesn't buy us much, other than adding boilerplate code around every Reporter.WriteLine callsite. It is easier to have a Reporter that just does nothing when called. A potentially better design would be to just have a global Reporter we just switch the operating mode when needed. That design is your Boolean idea but it is a global "lightswitch" for reporting as opposed to a Boolean that only works for these specific commands and classes. However, that feels like a bigger endeavor than is needed to solve this particular scenario. My heart yearns for a rewrite, but it isn't a priority at the moment.

[Forgind]

Thanks for the explanation! So I guess the only case in which we'd have to roll back part of this change would be if we decided to add text to one of those subcommands...which seems unlikely.

On the global boolean idea, it might be interesting to have an optional argument on WriteLine that, if false (default: true), would only write the line if the global boolean is true. I do think that would be a nice, clean structure and very forward-compatible, but I think it's fine to skip it for now since you said it isn't a priority 🙂

[Forgind]

Looks good to me! I was thinking about proposing either unifying the new NuGetPackageDownloader calls in one place, but I don't think that's really worth it. You can't just do it in place of the boolean unless you want it for commands that don't need it, which would be wasteful.

[MiYanni]

Breaking change issue for dotnet/docs created here: dotnet/docs#37278