-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[main] Update dependencies from dotnet/command-line-api #29131
[main] Update dependencies from dotnet/command-line-api #29131
Conversation
…uild 20221114.2 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.356402
…uild 20221118.1 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.356801
@adamsitnik could you help with updating CLI too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently introducing a lot of small breaking changes to System.CommandLine and I would prefer to merge the updates in larger batches, otherwise we have to update both SDK and runtime few times a week.
I've assigned the PR to myself and applied NO MERGE label. In a week/two I am going to solve all the issues and merge it.
@adamsitnik how are the breaking changes coming along? This is our oldest outstanding codeflow PR so popped up on my radar. |
…uild 20221213.1 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.361301
@adamsitnik @jeffhandley as we are fast approaching Preview 1 code-complete, can we please work on resolving the breaking changes and get this in? |
@ViktorHofer we are still not done with introducing breaking changes to S.CL. There were no bug fixes/new features merged in the meantime and there are multiple other repos depending on S.CL which could get easily broken after updating the version used by SDK. So I would prefer to defer this to Preview 2. |
@ViktorHofer / @dotnet/area-infrastructure-libraries - is there something we should do to turn this flow off until @adamsitnik gives us the all-clear? Or is it better practice to keep this PR open for another month? |
@MichaelSimons @mmitche is it possible to even build the product without taking this dependency update? Wouldn't we be in an incoherent state? I bet this would at least block source build, right? |
Have these breaking changes flowed into any other repos yet? @ViktorHofer is right in that the build has to be coherent in regards to the version of S.CL referenced. Everyone has to be on the same version. This is runtime, sdk, format, templating, and sourcelink. Did I miss any? |
…uild 20230119.2 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.406902
* OptionResult.Tokens returns Tokens of all its children * only CommandResult defines Children property * don't search for ParseError more than once
…y for Option<T>, to avoid type mismatches (example: Option<int> using default value as "abc")
# Conflicts: # src/Cli/Microsoft.TemplateEngine.Cli/CliTemplateParameter.cs # src/Cli/dotnet/commands/dotnet-list/dotnet-list-package/ListPackageReferencesCommandParser.cs # src/Cli/dotnet/commands/dotnet-test/TestCommandParser.cs
@@ -150,7 +151,14 @@ internal Option GetOption(IReadOnlySet<string> aliases) | |||
if (!string.IsNullOrWhiteSpace(DefaultValue) | |||
|| (Type == ParameterType.String || Type == ParameterType.Choice) && DefaultValue != null) | |||
{ | |||
option.SetDefaultValue(DefaultValue); | |||
if (option is Option<string> stringOption) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SetDefaultValue
is now avilable only for Option<T>
(not the non-generic Option
), to avoid type mismatches (example: Option<int>
using default value "abc").
I was not 100% sure that it's never something else than Option<string>
here, so I've added the Debug.Fail
to throw during development and not throw in the release builds.
@@ -25,24 +26,15 @@ internal static class Extensions | |||
/// <summary> | |||
/// Checks if <paramref name="parseResult"/> contains an error for <paramref name="option"/>. | |||
/// </summary> | |||
internal static bool HasErrorFor(this ParseResult parseResult, Option option) | |||
internal static bool HasErrorFor(this ParseResult parseResult, Option option, [NotNullWhen(true)] out ParseError? error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also a perf optimization: previously we were iterating the errors up to three times, now it's only once and the caller does not need to search for the error again
@@ -79,7 +79,12 @@ internal void ValidateArgumentsAreNotUsed(CommandResult commandResult) | |||
|
|||
internal void ValidateOptionUsage(CommandResult commandResult, Option option) | |||
{ | |||
OptionResult? optionResult = commandResult.Parent?.Children.FirstOrDefault(symbol => symbol.Symbol == option) as OptionResult; | |||
if (commandResult.Parent is not CommandResult parentResult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A parent of CommandResult
can be only another CommandResult
. In case it's null it means it's RootCommand
.
@@ -83,13 +83,8 @@ public bool Equals(InvalidTemplateOptionResult? other) | |||
return this.Equals(other as object); | |||
} | |||
|
|||
internal static new InvalidTemplateOptionResult FromParseResult(TemplateOption option, ParseResult parseResult) | |||
internal static InvalidTemplateOptionResult FromParseError(TemplateOption option, ParseResult parseResult, ParseError error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perf: we don't need to search for the error if the caller can provide it
@@ -41,7 +41,8 @@ protected override Task<NewCommandStatus> ExecuteAsync(ListCommandArgs args, IEn | |||
|
|||
private void ValidateParentCommandArguments(CommandResult commandResult) | |||
{ | |||
var nameArgumentResult = commandResult.Children.FirstOrDefault(symbol => symbol.Symbol == ListCommand.NameArgument); | |||
var nameArgumentResult = commandResult.Children.FirstOrDefault( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolResult.Symbol
was removed, only ArgumentResult
provides ArgumentResult.Argument
@@ -212,7 +212,7 @@ private static IEnumerable<string> GetRunPropertyOptions(ParseResult parseResult | |||
var optionString = shorthand ? "-p" : "--property"; | |||
var options = parseResult.CommandResult.Children.Where(c => c.Token().Type.Equals(TokenType.Option)); | |||
var propertyOptions = options.Where(o => o.Token().Value.Equals(optionString)); | |||
var propertyValues = propertyOptions.SelectMany(o => o.Children.SelectMany(c => c.Tokens.Select(t=> t.Value))).ToArray(); | |||
var propertyValues = propertyOptions.SelectMany(o => o.Tokens.Select(t=> t.Value)).ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OptionResult.Tokens
returns Tokens of all its children:
@@ -146,7 +146,7 @@ private static string GetSymbolResultValue(ParseResult parseResult, SymbolResult | |||
} | |||
else if (symbolResult.Token().Type.Equals(TokenType.Command)) | |||
{ | |||
return symbolResult.Symbol.Name; | |||
return ((System.CommandLine.Parsing.CommandResult)symbolResult).Command.Name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it must be true based on the condition above
@@ -26,8 +26,8 @@ public List<ApplicationInsightsEntryFormat> AllowList(ParseResult parseResult, D | |||
if (_topLevelCommandNameAllowList.Contains(topLevelCommandNameFromParse)) | |||
{ | |||
var firstOption = parseResult.RootCommandResult.Children | |||
.FirstOrDefault(c => c.Token() != default && c.Token().Type.Equals(TokenType.Command))? | |||
.Children.FirstOrDefault(c => c is System.CommandLine.Parsing.CommandResult)?.Symbol.Name ?? null; | |||
.OfType<System.CommandLine.Parsing.CommandResult>().FirstOrDefault()? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only CommandResult defines Children property now
|
||
public static Func<CompletionContext, IEnumerable<CompletionItem>> TargetFrameworksFromProjectFile => | ||
(_context) => | ||
public static IEnumerable<CompletionItem> TargetFrameworksFromProjectFile(CompletionContext _) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these properties was allocating memory each time they were called. We can just define static methods rather than properties that return lambdas.
ArgumentHelpName = LocalizableStrings.CrashDumpTypeArgumentName, | ||
} | ||
.ForwardAsMany(o => new[] { "-property:VSTestBlameCrash=true", $"-property:VSTestBlameCrashDumpType={o}" }); | ||
result.AcceptOnlyFromAmong(new string[] { "full", "mini" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is no longer an extension method that returns the option (BCL requirement is that only builder types are allowed to do this)
No, this is the only repo that still has System.CommandLine auto-updates enabled. Anyway I am going to work on updating all of them, as it's a good source of feedback for me (so far I've discovered one bug). |
If we merge this without updating the others it will break source-build. We should coordinate merging this PR with updating the other repos. |
…uild 20230606.1 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.430601
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of the most important code changes:
- many types have been renamed and they have
Cli
prefix now. This should allow us to avoid naming conflicts. - the symbol types no longer expose
.AddXYZ
methods, they exposeXYZ
collections (typically lists), so we can performAdd
,Clear
,RemoveAt
and similar operations Command.AddGlobalOption
is gone, theCliOption
type is now exposingRecursive
property (previously there was an internalGlobal
property that the removed method was setting)InvocationContext
is gone, asParseResult
is all we need for invocation.CancellationToken
is now being passed to all async actions, if we don't propagate it, we get a compiler warning.SymbolResult
exposesAddError
which allows us to report more than one error (previously it was a mutableErrorMessage
property which was limiting us to only one parse error per symbol)IConsole
was removed, the config is now exposingOutput
andError
which are both justTextWriter
s- the non-generic overload for setting default value was removed, as it was allowing for introducing silent bugs. Example: defining an
CliOption<int>
and passingbool
as default value. ICommandHandler
has been replaced withCliAction
LocalizationResources
are gone, the resources from SDK were merged into S.CL resources (see Localization command-line-api#2041 for details).Middleware
is gone, it was expensive, complex and almost not used at all.- the
CliOption
ctor accepts(string name, params string[] aliases)
. It required to update plenty of usages, but gives us a clear separation betweeenName
andAliases
. PreviouslyName
was the longest alias without prefix. Now it needs to be provided in explicit way and it's not included in theAlises
collection.
This introduces the only breaking change:
So far, for every option the help was printing name, aliases and name without prefix or help name when it was provided. Now we don't print name without prefix right:
- --name, --alias1, -a <name> description
+ --name, --alias1, -a description
Sample diff:
I've reviewed all my changes and added comments in places where they might not seem obvious.
@baronfel PTAL, the PR is ready for review now.
@@ -149,7 +150,34 @@ internal Option GetOption(IReadOnlySet<string> aliases) | |||
if (!string.IsNullOrWhiteSpace(DefaultValue) | |||
|| (Type == ParameterType.String || Type == ParameterType.Choice) && DefaultValue != null) | |||
{ | |||
option.SetDefaultValue(DefaultValue); | |||
switch (option) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the non-generic overload for setting default value was removed, as it was allowing for introducing silent bugs. Example: defining an CliOption<int>
and passing bool
as default value.
I know that this switch is far from perfect, I am open to suggestions about how it can be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably ok for the template engine - it has a relatively constrained set of types that are allowable as part of the grammar for template options (choice, string, bool, int, etc), and this just becomes an explicit mapping that needs to occur.
@dotnet/templating-engine-maintainers can you take a look and verify that all of our data types are manually mapped here now?
|
||
namespace Microsoft.TemplateEngine.Cli.Commands | ||
{ | ||
internal partial class InstantiateCommand | ||
{ | ||
private const string Indent = " "; | ||
private static Lazy<ResourceManager> _resourceManager = new Lazy<ResourceManager>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LocalizationResources
are no longer public, the resources from SDK were merged into S.CL resources (see dotnet/command-line-api#2041 for details). To read the 4 resources that were actually used by SDK I've decided to use ResourceManager
to read S.CL
resources. In the long term, we should remove it.
|
||
// Argument | ||
public static readonly Argument<string> DotnetSubCommand = new Argument<string>() { Arity = ArgumentArity.ExactlyOne, IsHidden = true }; | ||
public static readonly CliArgument<string> DotnetSubCommand = new("subcommand") { Arity = ArgumentArity.ZeroOrOne, Hidden = true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously this argument was marked as ArgumentArity.ExactlyOne
which was making it a required argument. When it was not provided, a parse error was being reported. Since it's not required, I've changed it to optional (ArgumentArity.ExactlyOne
), so fewer workarounds are required elsewhere (like not ignoring certain parse errors)
|
||
return rootCommand; | ||
} | ||
rootCommand.SetAction(parseResult => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so far the root command had no explicit handler, but in reality it had some defined in Program.cs
. I've moved this logic here and used the built-in mechanism for that.
It was my own review that I've used to prevent someone from merging it by accident
{ | ||
var quietOption = new Option<bool>(new[] { "--quiet", "-q" }, "Suppresses all output except warnings and errors"); | ||
var verboseOption = new Option<bool>(new[] { "--verbose", "-v" }, "Show verbose output"); | ||
CliOption<bool> quietOption = new("--quiet", "-q") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please keep the previous style, i.e. var _ = new CliOption<bool>(...)
instead of CliOption<bool> _ = new(...)
? It'd be a) easier to read the PR and b) consistent with https://github.com/dotnet/sdk/pull/29131/files#diff-48c696e38dedfa782c9bb14f1e5749d861c4cd6ae825edda9497bb62deebdc3cR146
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmat I apology, I should not bring my var
obsession from dotnet/runtime
to this repo.
Changing that is possible, but it would take me quite a lot of time (I could automate it only partially) for the entire solution. Unless you are concerned about only this particular file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just this particular file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmat done, PTAL
-n, --name <name> The name for the output being created. If no name is specified, the name of the output directory is used. | ||
-o, --output <output> Location to place the generated output. | ||
--dry-run Displays a summary of what would happen if the given command line were run if it would result in a template creation. | ||
--force Forces content to be generated even if it would change existing files. | ||
--no-update-check Disables checking for the template package updates when instantiating a template. | ||
--project <project> The project that should be used for context evaluation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for these kinds of diffs, we should supply and HelpName if we want to keep this layout, correct @adamsitnik?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for these kinds of diffs, we should supply and HelpName if we want to keep this layout, correct
That is correct. Would you like me to add the HelpName
values and keep the old layout?
@@ -504,7 +503,7 @@ internal void CanParseMultiChoiceTemplateOptions(string command, string paramete | |||
//new object?[] { "foo --framework", "framework", "net5.0|net6.0", false, null, null, "Required argument missing for option: '--framework'." }, | |||
//requireness is no longer set on parser level | |||
//new object?[] { "foo", "framework", "net5.0|net6.0", true, null, null, "Option '--framework' is required." }, | |||
new object?[] { "foo --framework", "framework", "net5.0|net6.0", true, null, "netcoreapp2.1", $"Cannot parse default if option without value 'netcoreapp2.1' for option '--framework' as expected type 'choice': value 'netcoreapp2.1' is not allowed, allowed values are: 'net5.0','net6.0'. Did you mean one of the following?{Environment.NewLine}net5.0{Environment.NewLine}net6.0" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did levenstein get removed from suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case, the error message got simplified.
Before the changes the first sentence was finished with:
allowed values are: 'net5.0','net6.0'
Then in the next one the information was repeated (in a way, the meaning was the same):
Did you mean one of the following?{Environment.NewLine}net5.0{Environment.NewLine}net6.0
I forgot to mention that the S.CL update brings some perf improvements. In case of things like For other things that include calling But such huge gains should not be expected for all scenarios, for example for
|
…ae-4e82-ad57-55ddf2973d18 # Conflicts: # src/ApiCompat/Microsoft.DotNet.ApiCompat.Tool/Program.cs
…ae-4e82-ad57-55ddf2973d18 # Conflicts: # src/Containers/containerize/ContainerizeCommand.cs # src/Tests/containerize.UnitTests/ParserTests.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help changes look good to me, that really reduced the diffs. Thanks @adamsitnik!
…uild 20230607.1 Microsoft.SourceBuild.Intermediate.command-line-api , System.CommandLine From Version 0.1.356401 -> To Version 0.1.430701
* nuget command forwards some subcommands to nuget app, so it needs to define TreatUnmatchedTokensAsErrors=false * revert the test change to match old help look
Friendly reminder that this checkin is likely going to block the flow into installer until all of repos that depend on commond-line-api will build with this new version. |
@MichaelSimons I've opened a revert PR so we can do that coordination before trying again. After checking darc we'll need to synchronize:
|
@baronfel in what order? |
@adamsitnik source build uses a single version so I think that all five need to be coordinated and go in roughly the same time. They'll be fine as the flow until they get to the installer repo so codeflow to installer will be blocked until all five have flowed. |
This pull request updates the following dependencies
From https://github.com/dotnet/command-line-api