update System.CommandLine to the latest version #68549
Conversation
stop using System.CommandLine.NamingConventionBinder as it's reflection based and won't be supported in the long term stop using ancient System.CommandLine.Experimental
| <add key="dotnet8" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json" /> | ||
| <add key="dotnet8-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json" /> | ||
| <add key="dotnet-public" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json" /> | ||
| <add key="dotnet-libraries" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-libraries/nuget/v3/index.json" /> |
There was a problem hiding this comment.
Nine different dotnet feeds now ... I feel like we're making NuGet unnecessarily complicated.
|
|
||
| var parser = CreateCommandLineParser(); | ||
| return await parser.InvokeAsync(args); | ||
| return await parser.Parse(args).InvokeAsync(CancellationToken.None); |
There was a problem hiding this comment.
What cancellation token should be going in here? Was the old behavior listening to Ctrl+C to raise the token or not?
There was a problem hiding this comment.
The docs show InvokeAsync with no cancellationtoken. Might be one of our analyzers telling it to pass a cancellation token?
https://learn.microsoft.com/en-us/dotnet/standard/commandline/handle-termination
There was a problem hiding this comment.
Whatever token we pass here, S.CL is going to create a source linked to it:
and create it's own token out of it, then request the cancellation when its Ctrl+C is pressed
jasonmalinowski
left a comment
There was a problem hiding this comment.
Generally looks OK, some questions about cancellation tokens but that could be done as a follow-up.
| var logLevelOption = new CliOption<LogLevel>("--logLevel") | ||
| { | ||
| var value = result.Tokens.Single().Value; | ||
| return !Enum.TryParse<LogLevel>(value, out var logLevel) |
There was a problem hiding this comment.
@dibarbet Were enums just not supported on the version we were on or was this necessary for some special reason?
There was a problem hiding this comment.
I honestly don't remember. If this works with VSCode then I'm good with it.
There was a problem hiding this comment.
enums are now supported OOTB, I can not speak about the past as I've been involved in this project only for a year and a half
| cancellationToken); | ||
| }); | ||
|
|
||
| return generateCommand.Parse(args).InvokeAsync(CancellationToken.None); |
There was a problem hiding this comment.
Same question as earlier: what should go here for the token?
There was a problem hiding this comment.
Same question as earlier: what should go here for the token?
If this is Main, then None.
If this was any other async method that would have it's own token, then that token.
stop using System.CommandLine.NamingConventionBinder as it's reflection based and won't be supported in the long term
stop using ancient System.CommandLine.Experimental
Contributes to dotnet/sdk#33157