stop using System.CommandLine types that won't be public anymore #5493
Conversation
| @@ -199,7 +199,7 @@ public string GetDetailedHelp(string commandName, IServiceProvider services, int | |||
| { | |||
| if (handler.IsCommandSupported(group.Parser, services)) | |||
| { | |||
| return group.GetDetailedHelp(command, services, consoleWidth); | |||
| return group.GetDetailedHelp(command, services); | |||
There was a problem hiding this comment.
I've not removed the int consoleWidth argument from the public method as I am not sure what public means in this context ( is it exposed as a library to external customers who would get build errors?)
There was a problem hiding this comment.
Leaving the consoleWidth parameter in the public function is ok for now. There are other references outside of this repo.
adamsitnik
force-pushed
the
sclHelpIsInternal
branch
from
089db72 to
53c7036
Compare
| { | ||
| StringWriter console = new(); | ||
| CommandLineConfiguration configuration = new(command) |
There was a problem hiding this comment.
Removing the consoleWidth parameter means that the command help doesn't get formatted for the console width anymore? Hopefully System.CommandLine isn't using Console.WIndowWidth directly because this code can get hosted in an environment that doesn't work.
There was a problem hiding this comment.
It still does get formatted (when the output is not redirected):
There was a problem hiding this comment.
Yes, but it is using Console.WindowWidth which doesn't work when SOS is hosted under windbg or lldb. The hosting infrastructure has a method to get the console width that is passed down to the CommandService.
There was a problem hiding this comment.
Yes, but it is using Console.WindowWidth which doesn't work when SOS is hosted under windbg or lldb. The hosting infrastructure has a method to get the console width that is passed down to the CommandService.
Do you remember if it just throws for SOS? If so, would it be acceptable to just wrap the logic in System.CommandLine and use int.MaxValue when Console.Width throws?
The alternative would be to extend the public HelpAction (or HelpOption) class with a new public property that would allow the users to specify the width (cc @jonsequitur)
There was a problem hiding this comment.
@mikem8361 Do what the width is used for in SOS when it is hosted under windbg/lldb? I am wondering if we could remove the width dependency and have fixed formatting when it is hosted.
There was a problem hiding this comment.
The problem is that this new version of System.CommandLine now calls Console.WindowWidth and SOS can't do anything about it. I'm not sure the API would actually throw an exception under the native debuggers but it does NOT return the correct screen width.
It is System.CommandLine that formats the help text given the screen width. In the current version that SOS uses, SOS can pass the screen width to the System.CommandLine help formatting function but the new version uses Console.WindowWidth directly.
There was a problem hiding this comment.
The alternative would be to extend the public HelpAction (or HelpOption) class with a new public property that would allow the users to specify the width (cc @jonsequitur)
These kinds of properties shouldn't be on HelpOption because it assumes that the associated action is going to do something with the console at all, and I don't think that's always true. It would be more appropriate on HelpAction, though more awkward to get to. But we should definitely preserve the ability for the formatting to work correctly when console output is redirected.
Context: dotnet/command-line-api#2563