WIP: Add name argument for dotnet new #33810

MiYanni · 2023-07-05T23:08:12Z

This is attempting to add a name argument for dotnet new when creating a template. There is already an existing --name option, but there was a suggestion to use a name argument (value with no --{something} identifier). It should only be positional and exist after the template name.

I ran into several issues with this as the tests make assumptions about the code flow and parsing of the commands. The current tests I was looking at, Create_CanDetectParseErrorsTemplateOptions, essentially recreates the code flow of activating the command (like when a user invokes it) but doesn't run down that code path. It has manual parse calls which end up not realistically representing the code flow.

After making these changes, 70+ tests were broken and I've fixed several in this code so far. Not sure if I should keep going down this rabbit hole with the implementation presented here.

…pi (dotnet#29131)""

… A bunch of adjustments were necessary to make help and flow the name properly to the template command. Other adjustments made to satisfy testing situations. In-progress of updating tests to run properly.

…on making tests run properly.

# Conflicts: # src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.Legacy.cs # src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.cs # src/Cli/Microsoft.TemplateEngine.Cli/Commands/SharedOptions.cs # src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.Help.cs # src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs # src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/TemplateCommand.cs # src/Containers/containerize/ContainerizeCommand.cs # src/Tests/Microsoft.TemplateEngine.Cli.UnitTests/ParserTests/InstantiateTests.cs

…s for context.

MiYanni · 2023-07-05T23:09:11Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/SharedOptions.cs

+        internal static CliArgument<string> NameArgument { get; } = new CliArgument<string>("name")
+        {
+            Description = SymbolStrings.TemplateCommand_Option_Name,
+            Arity = new ArgumentArity(0, 1)


Even though this is a shared name 'argument', it is used in the code the same way as the rest of the shared options.

MiYanni · 2023-07-05T23:12:22Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs

+
+#pragma warning disable CA1826
+            var firstTemplateCommandToken = parseResult.CommandResult.Tokens.FirstOrDefault()?.Value ?? string.Empty;
+#pragma warning restore CA1826


Not sure why I was getting a CA1826 here as the logic requires FirstOrDefault since I'm not sure if there even is a token available.

MiYanni · 2023-07-05T23:13:21Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs

-                CliConfiguration parser = ParserFactory.CreateParser(command);
-                ParseResult parseResult = parser.Parse(args.RemainingArguments ?? Array.Empty<string>());
+                ParseResult parseResult = Reparse(command, args.RemainingArguments ?? Array.Empty<string>());


You no longer need to create a parser out of the factory and sent the command to it. The Parse method on the command (which may or may not be an extension method) does this already.

MiYanni · 2023-07-05T23:13:58Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.cs

+        // This is only used to let the help generation know there is a name argument.
+        // It doesn't contribute to parsing the arguments because the arity is set to 0.
+        // InstantiateCommand reparses the arguments in the context of TemplateCommand to properly parse the name.
+        internal static CliArgument<string> NameArgument { get; } = new CliArgument<string>("name")
+        {
+            Description = SymbolStrings.TemplateCommand_Option_Name,
+            Arity = new ArgumentArity(0, 0),
+            Hidden = true
+        };


Workaround to make help generate properly.

probably we can just adapt the help generation instead, as it is already custom.

see here:

sdk/src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.Help.cs

Line 462 in 0ba5815

private static void CustomUsageSection(HelpContext context, CliCommand command)

Did some investigation with this. It looks like both dotnet new --help and dotnet new create --help both call this CustomUsageSection method. Without my code above, new is missing the name argument information, but create is not. This is because InstantiateCommand (aka create) has the real name argument, but we don't want a real name argument for NewCommand. We only want to show it in the help for new.

The CustomUsageSection method is for a custom Usage section but doesn't cover the Arguments section, which is currently not custom. That's why I opted to create this 'fake' name argument since it would "just work" for both sections. If I was to touch this help output, I'd likely refactor more than just working around this situation. But I didn't want to do all that for this PR.

MiYanni · 2023-07-05T23:16:46Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs

+            if (firstTemplateCommandToken.StartsWith('-') && (tokenIndex = argsList.IndexOf(firstTemplateCommandToken)) >= 0)
+            {
+                argsList.Insert(tokenIndex, TemplateCommandArgs.NameDefaultSentinel);
+                parseResult = command.Parse(argsList, configuration);


This solution does require this second parse as you need to have the tokens individually first; then use the sentinel if the first token isn't supposed to be name.

MiYanni · 2023-07-05T23:19:54Z

src/Tests/Microsoft.TemplateEngine.Cli.UnitTests/ParserTests/InstantiateTests.cs

+            var tokensToInvoke = args.TokensToInvoke ?? Array.Empty<string>();
+            if (tokensToInvoke.Last() == "foo")
+            {
+                tokensToInvoke = tokensToInvoke.SkipLast(1).ToArray();
+            }
+            ParseResult templateParseResult = parser.Parse(tokensToInvoke);


This is fun as foo would previously be 'unparsed' and the test would run properly. Now, it was getting picked up as the name argument and thus having 2 assigned names, which we've made into an error.

MiYanni · 2023-07-05T23:20:43Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/TemplateCommandArgs.cs

+            var nameOption = parseResult.GetValueForOptionOrNull(SharedOptions.NameOption);
+            if (nameArgument is not null && nameOption is not null)
+            {
+                throw new InvalidOperationException($"Name argument '{nameArgument}' and name option (--name) '{nameOption}' have both been provided. Only one may be provided at a time.");


Is this the right way to error here? I've seen some throws like this, but it would also mean that these situations are unlocalized.

it is better to do it via validation delegate instead.
See as example:

sdk/src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.Legacy.cs

Line 47 in 0ba5815

internal void AddNoLegacyUsageValidators(CliCommand command, params CliSymbol[] except)

I've added a validator for this situation. When are the validators executed?

MiYanni · 2023-07-05T23:23:46Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs

@@ -172,6 +173,26 @@ async Task<string> GetTemplatePackagesList(TemplateGroup templateGroup)
            }
        }

+        // Reparses the CLI parameters in the context of the provided command (usually TemplateCommand).
+        // If the first token after reparsing starts with a dash, we reparse again and inject a name sentinel to allow it to be handled properly (using empty name logic).
+        internal static ParseResult Reparse(CliCommand command, IReadOnlyList<string> args, CliConfiguration? configuration = null)


The reason this solution works at all is because when we go to call a template, we reparse in the context of the template command. Otherwise, the name argument would always be lumped into the template options. If you debug here, you'll see the name argument value come out of the template options bucket and go into the appropriate name argument in TemplateCommand.

I would expect parser should be capable to do a bit of magic for us already on the first pass.
I would expect it should be possible to achieve same via parseArgument delegates set on the Name and RemainingArgs.

The logic for name:

check if argument starts with -: yes - this is an arg, parse as null, no - this is a name.

The logic for remaining args (if needed)

if name was an arg, prepend it as the first arg

Reparsing is costly operation and in this case it might be avoided, also it streamlines the logic.

Example of custom parseArgument delegate is here:

sdk/src/Containers/containerize/ContainerizeCommand.cs

Lines 94 to 152 in 0ba5815

internal CliOption<Port[]> PortsOption { get; } = new("--ports")

{

Description = "Ports that the application declares that it will use. Note that this means nothing to container hosts, by default - it's mostly documentation. Ports should be of the form {number}/{type}, where {type} is tcp or udp",

AllowMultipleArgumentsPerToken = true,

CustomParser = result =>

{

string[] ports = result.Tokens.Select(x => x.Value).ToArray();

var goodPorts = new List<Port>();

var badPorts = new List<(string, ContainerHelpers.ParsePortError)>();

foreach (string port in ports)

{

string[] split = port.Split('/');

if (split.Length == 2)

{

if (ContainerHelpers.TryParsePort(split[0], split[1], out var portInfo, out var portError))

{

goodPorts.Add(portInfo.Value);

}

else

{

var pe = (ContainerHelpers.ParsePortError)portError!;

badPorts.Add((port, pe));

}

}

else if (split.Length == 1)

{

if (ContainerHelpers.TryParsePort(split[0], out var portInfo, out var portError))

{

goodPorts.Add(portInfo.Value);

}

else

{

var pe = (ContainerHelpers.ParsePortError)portError!;

badPorts.Add((port, pe));

}

}

else

{

badPorts.Add((port, ContainerHelpers.ParsePortError.UnknownPortFormat));

continue;

}

}

if (badPorts.Count != 0)

{

var builder = new StringBuilder();

builder.AppendLine("Incorrectly formatted ports:");

foreach (var (badPort, error) in badPorts)

{

var errors = Enum.GetValues<ContainerHelpers.ParsePortError>().Where(e => error.HasFlag(e));

builder.AppendLine($"\t{badPort}:\t({string.Join(", ", errors)})");

}

result.AddError(builder.ToString());

return Array.Empty<Port>();

}

return goodPorts.ToArray();

}

};

Some clarification/comments:

Reparsing was already taking place here. I ended up extracting the reparsing logic out into this method and calling it in 2 places.

Changing out how RemainingArgs works changes the entirety of dotnet new <TemplateName> parsing, including the arguments specific to a template. I consider that out-of-scope for this PR. That should get easier to do the more familiar with S.CL I become.

I'll give the custom parser a try specifically for Name. As long as it is being activated during reparsing, it should work. But I need to validate that first.

@vlada-shubina I did some testing here and was unsuccessful with using CustomParser. The situation is quite complex and I took a deeper dive into it. I found that currently, the dotnet new <TEMPLATE> command is reparsed many times. In a simple dotnet new console scenario (using the code from main, not from this PR), it ends up reparsing 6 times. I recorded a video for you showing what happens (only viewable by MSFT).

I need to double check if the condition I put below to reparse for the name situation is called more than once. If it is, then I need to change it.

vlada-shubina

I left some comments inline.

I hope the suggested approach helps with tests too, if not, let's discuss it separately.

src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.Legacy.cs

vlada-shubina · 2023-07-10T10:52:40Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.cs

+        // This is only used to let the help generation know there is a name argument.
+        // It doesn't contribute to parsing the arguments because the arity is set to 0.
+        // InstantiateCommand reparses the arguments in the context of TemplateCommand to properly parse the name.
+        internal static CliArgument<string> NameArgument { get; } = new CliArgument<string>("name")
+        {
+            Description = SymbolStrings.TemplateCommand_Option_Name,
+            Arity = new ArgumentArity(0, 0),
+            Hidden = true
+        };


probably we can just adapt the help generation instead, as it is already custom.

vlada-shubina · 2023-07-10T10:53:53Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.cs

+        // This is only used to let the help generation know there is a name argument.
+        // It doesn't contribute to parsing the arguments because the arity is set to 0.
+        // InstantiateCommand reparses the arguments in the context of TemplateCommand to properly parse the name.
+        internal static CliArgument<string> NameArgument { get; } = new CliArgument<string>("name")
+        {
+            Description = SymbolStrings.TemplateCommand_Option_Name,
+            Arity = new ArgumentArity(0, 0),
+            Hidden = true
+        };


see here:

sdk/src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.Help.cs

Line 462 in 0ba5815

private static void CustomUsageSection(HelpContext context, CliCommand command)

vlada-shubina · 2023-07-10T10:56:34Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/TemplateCommandArgs.cs

+            var nameOption = parseResult.GetValueForOptionOrNull(SharedOptions.NameOption);
+            if (nameArgument is not null && nameOption is not null)
+            {
+                throw new InvalidOperationException($"Name argument '{nameArgument}' and name option (--name) '{nameOption}' have both been provided. Only one may be provided at a time.");


it is better to do it via validation delegate instead.
See as example:

sdk/src/Cli/Microsoft.TemplateEngine.Cli/Commands/NewCommand.Legacy.cs

Line 47 in 0ba5815

internal void AddNoLegacyUsageValidators(CliCommand command, params CliSymbol[] except)

vlada-shubina · 2023-07-10T13:14:35Z

src/Cli/Microsoft.TemplateEngine.Cli/Commands/create/InstantiateCommand.cs

@@ -172,6 +173,26 @@ async Task<string> GetTemplatePackagesList(TemplateGroup templateGroup)
            }
        }

+        // Reparses the CLI parameters in the context of the provided command (usually TemplateCommand).
+        // If the first token after reparsing starts with a dash, we reparse again and inject a name sentinel to allow it to be handled properly (using empty name logic).
+        internal static ParseResult Reparse(CliCommand command, IReadOnlyList<string> args, CliConfiguration? configuration = null)


I would expect parser should be capable to do a bit of magic for us already on the first pass.
I would expect it should be possible to achieve same via parseArgument delegates set on the Name and RemainingArgs.

The logic for name:

check if argument starts with -: yes - this is an arg, parse as null, no - this is a name.

The logic for remaining args (if needed)

if name was an arg, prepend it as the first arg

Reparsing is costly operation and in this case it might be avoided, also it streamlines the logic.

…tion. Resourcized this error message.

…ed the 3 arguments from NewCommand.Legacy to NewCommand. Rebuilt so the missing localization strings are present.

MiYanni · 2023-09-20T21:06:29Z

.editorconfig

@@ -297,7 +297,7 @@ dotnet_diagnostic.CA1823.severity = warning
 # Avoid zero-length array allocations.
 dotnet_diagnostic.CA1825.severity = warning
 # Do not use Enumerable methods on indexable collections. Instead use the collection directly
-dotnet_diagnostic.CA1826.severity = warning
+dotnet_diagnostic.CA1826.severity = none


Disabled this warning as there are specific situations in which you want to use .FirstOrDefault() since you cannot guarantee the collection has any elements to access directly without manual checks/extra boilerplate code.

marcpopMSFT and others added 7 commits June 9, 2023 14:03

Revert "Revert "[main] Update dependencies from dotnet/command-line-a…

7962096

…pi (dotnet#29131)""

Merge branch 'main' into revert-33146-revert-scl-update

130d51a

Fix up merge errors

7acdadb

Changed the name option into a name argument when creating templates.…

d2ae573

… A bunch of adjustments were necessary to make help and flow the name properly to the template command. Other adjustments made to satisfy testing situations. In-progress of updating tests to run properly.

Adjustments based on shared code. Remove commented code. In-progress …

8bd9407

…on making tests run properly.

Fix incorrect rebase change (ContainerizeCommand). Added more comment…

5810d5a

…s for context.

MiYanni added the Area-Templates label Jul 5, 2023

dotnet-issue-labeler bot added the untriaged Request triage from a team member label Jul 5, 2023

MiYanni commented Jul 5, 2023

View reviewed changes

MiYanni requested review from baronfel and vlada-shubina July 5, 2023 23:21

MiYanni commented Jul 5, 2023

View reviewed changes

vlada-shubina reviewed Jul 10, 2023

View reviewed changes

MiYanni added 3 commits September 15, 2023 17:07

Merge branch 'main' into NameAsArgumentForTemplates

e923083

Added a validator to TemplateCommandArgs instead of throwing an excep…

a093ee3

…tion. Resourcized this error message.

Disabled the CA1826 warning since it is not particularly helpful. Mov…

7553608

…ed the 3 arguments from NewCommand.Legacy to NewCommand. Rebuilt so the missing localization strings are present.

MiYanni commented Sep 20, 2023

View reviewed changes

MiYanni mentioned this pull request Dec 22, 2023

CLI - allow second parameter to specify name of project/item template dotnet/templating#3009

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add name argument for dotnet new #33810

WIP: Add name argument for dotnet new #33810

MiYanni commented Jul 5, 2023

MiYanni Jul 5, 2023

MiYanni Jul 5, 2023

MiYanni Jul 5, 2023

MiYanni Jul 5, 2023

vlada-shubina Jul 10, 2023

vlada-shubina Jul 10, 2023

MiYanni Sep 16, 2023

MiYanni Jul 5, 2023

MiYanni Jul 5, 2023

MiYanni Jul 5, 2023

vlada-shubina Jul 10, 2023 •

edited

Loading

MiYanni Sep 16, 2023

MiYanni Jul 5, 2023

vlada-shubina Jul 10, 2023

vlada-shubina Jul 10, 2023

MiYanni Sep 18, 2023

MiYanni Sep 20, 2023

vlada-shubina left a comment

vlada-shubina Jul 10, 2023

vlada-shubina Jul 10, 2023

vlada-shubina Jul 10, 2023 •

edited

Loading

vlada-shubina Jul 10, 2023

MiYanni Sep 20, 2023

	internal CliOption<Port[]> PortsOption { get; } = new("--ports")
	{
	Description = "Ports that the application declares that it will use. Note that this means nothing to container hosts, by default - it's mostly documentation. Ports should be of the form {number}/{type}, where {type} is tcp or udp",
	AllowMultipleArgumentsPerToken = true,
	CustomParser = result =>
	{
	string[] ports = result.Tokens.Select(x => x.Value).ToArray();
	var goodPorts = new List<Port>();
	var badPorts = new List<(string, ContainerHelpers.ParsePortError)>();

	foreach (string port in ports)
	{
	string[] split = port.Split('/');
	if (split.Length == 2)
	{
	if (ContainerHelpers.TryParsePort(split[0], split[1], out var portInfo, out var portError))
	{
	goodPorts.Add(portInfo.Value);
	}
	else
	{
	var pe = (ContainerHelpers.ParsePortError)portError!;
	badPorts.Add((port, pe));
	}
	}
	else if (split.Length == 1)
	{
	if (ContainerHelpers.TryParsePort(split[0], out var portInfo, out var portError))
	{
	goodPorts.Add(portInfo.Value);
	}
	else
	{
	var pe = (ContainerHelpers.ParsePortError)portError!;
	badPorts.Add((port, pe));
	}
	}
	else
	{
	badPorts.Add((port, ContainerHelpers.ParsePortError.UnknownPortFormat));
	continue;
	}
	}

	if (badPorts.Count != 0)
	{
	var builder = new StringBuilder();
	builder.AppendLine("Incorrectly formatted ports:");
	foreach (var (badPort, error) in badPorts)
	{
	var errors = Enum.GetValues<ContainerHelpers.ParsePortError>().Where(e => error.HasFlag(e));
	builder.AppendLine($"\t{badPort}:\t({string.Join(", ", errors)})");
	}
	result.AddError(builder.ToString());
	return Array.Empty<Port>();
	}
	return goodPorts.ToArray();
	}
	};

WIP: Add name argument for dotnet new #33810

Are you sure you want to change the base?

WIP: Add name argument for dotnet new #33810

Conversation

MiYanni commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlada-shubina Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlada-shubina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlada-shubina Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlada-shubina Jul 10, 2023 •

edited

Loading

vlada-shubina Jul 10, 2023 •

edited

Loading