fix(#45761): Stop forwarding dotnet-watch run arguments to all commands #50558
Conversation
ronald-mz
changed the title
|
@dotnet-policy-service agree |
ronald-mz
changed the title
| foreach (var child in parseResult.CommandResult.Children) | ||
| { | ||
| var optionResult = (OptionResult)child; | ||
|
|
||
| // skip watch options: | ||
| if (!watchOptions.Contains(optionResult.Option)) |
There was a problem hiding this comment.
Inverting the logic allows a continue to short-circuit the loop.
| } | ||
|
|
||
| // skip forwarding run options to other commands. | ||
| if (isRunCommand == false && forwardToRunOptions.Contains(optionResult.Option)) |
There was a problem hiding this comment.
The meat and potatoes of this PR. Prevents options which only work with the run command from being passed to other commands.
There was a problem hiding this comment.
Can we pass command from the caller and generalize this to if (forwardToSubcommandOptions.Contains(optionResult.Option) && !command.Options.Contains(optionResult.Option)) ?
There was a problem hiding this comment.
Of course. Give me a bit and I'll get some commits up.
Further Thoughts
This is outside the scope of this fix, but a different PR could go a step further towards generalization by:
nixing forwardToSubcommandOptions in favor of watchableSubCommandOptions.
watchableSubCommandOptions are options that can show up in subcommands that could affect watch's settings.
For example: run --project and build [<PROJECT>|<SOLUTION>] could both set watch's project path.
| @@ -61,6 +61,12 @@ internal sealed class CommandLineOptions | |||
| } | |||
| }); | |||
|
|
|||
| // Options we need to know about that are passed through to the subcommand: | |||
There was a problem hiding this comment.
add ... if the subcommand supports them
| var longProjectOption = new Option<string>("--project") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false }; | ||
| var launchProfileOption = new Option<string>("--launch-profile", "-lp") { Hidden = true, Arity = ArgumentArity.ZeroOrOne, AllowMultipleArgumentsPerToken = false }; | ||
| var noLaunchProfileOption = new Option<bool>("--no-launch-profile") { Hidden = true, Arity = ArgumentArity.Zero }; | ||
| Option[] forwardToRunOptions = |
There was a problem hiding this comment.
I'd call this forwardToSubcommandOptions
There was a problem hiding this comment.
Nit: You can replace https://github.com/dotnet/sdk/pull/50558/files#diff-6a6fa2fea5af87fda145d79165f529cef772a3227d58494098b7a22511c4aecaR96-R99 with
foreach (var option in forwardToSubcommandOptions)
{
rootCommand.Options.Add(option);
}
Fixes: #45761, #45634, and maybe a regression of #34949.
Context
Watch CLI
Problem
dotnet-watchhas-p,--project,--launch-profile, and--no-launch-profileas possible arguments. A user can not use these arguments withoutdotnet-watchforwarding them. These arguments only work with theruncommand, and will cause an error if passed to any command butrun.