-
-
Notifications
You must be signed in to change notification settings - Fork 509
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
Command line options inherited from branch settings are not being bound. #1612
Comments
I'm experiencing the same issue. It seams that the derived options are not honored in the parent class. These are the settings classes. public class DefaultCustomerSettings : CommandSettings
{
[CommandOption("-o|--output <FORMAT>")]
[Description("Output format. Allowed values: json, jsonc, none, table, tsv, yaml, yamlc.")]
[DefaultValue(OutputFormat.json)]
public OutputFormat Output { get; set; }
}
public class CreateCustomerSettings : DefaultCustomerSettings
{
[CommandArgument(0, "<CUSTOMER_NAME>")]
public string CustomerName { get; set; }
} Then the command public override async Task<int> ExecuteAsync(CommandContext context, CreateCustomerSettings settings)
{
AnsiConsole.MarkupLine($"Creating new customer [[{settings.CustomerName}]].");
await _customerService.CreateNewCustomerAsync(settings.CustomerName);
AnsiConsole.MarkupLine("Done");
return 0;
} But the console does not show the options. And the value is always the default value. c:\Test> dotnet run -- customer create -h
DESCRIPTION:
Create customer the the customers table.
USAGE:
aztai customer create <CUSTOMER_NAME> [OPTIONS]
ARGUMENTS:
<CUSTOMER_NAME>
OPTIONS:
-h, --help Prints help information |
I found a workaround that will probably help someone find the root cause of the issue. Replace the properties inherited from your base CommandSettings class and forward them to it: public class BranchSettings : CommandSettings
{
[CommandOption("--my-value")]
public string MyValue { get; set; } = string.Empty;
}
public class BranchedCommandSettings : BranchSettings
{
[CommandOption("--my-value")]
public string MyValue
{
get => base.MyValue;
set => base.MyValue = value;
}
} Most likely the reflection code used by this library forgot to include the flag for inheritted members. |
I found the code responsible for this behavior, and it wasn't because of the reflection flag because that decision was intentional. spectre.console/src/Spectre.Console.Cli/Internal/Modelling/CommandModelBuilder.cs Lines 125 to 139 in cecfdc3
spectre.console/src/Spectre.Console.Cli/Internal/Modelling/CommandInfoExtensions.cs Lines 24 to 56 in cecfdc3
I think this is saying that if there's a parent command (aka branch), then the setting property will only be set it if the parent has the same settings type. I think we're all confused by this decision. This also may not have been their intent based on this comment: spectre.console/src/Spectre.Console.Cli/Internal/Modelling/CommandModelBuilder.cs Lines 91 to 94 in cecfdc3
|
Information
Describe the bug
A clear and concise description of what the bug is.
I believe this is the same bug as mentioned in #428, where command options from parent settings aren't being taken into account / parsed.
I'm trying to pass in command options that are stored in a parent settings class, but these command options all appear to be ignored.
And my program file:
To Reproduce
I have created a minimum reproducible example here: https://github.com/robcao/spectre-console-branch-inheritance
To reproduce, download the example, navigate to the
repro
directory, and rundotnet run
.The application fails with the following exception:
Expected behavior
A clear and concise description of what you expected to happen.
I expect command options defined on parent setting classes to be bound.
When running
dotnet run branch command --my-value abc
, I would expect the following:BranchedCommand .Execute
method, the value ofBranchedCommandSettings.MyValue
should beabc
.Screenshots
If applicable, add screenshots to help explain your problem.
Additional context
Add any other context about the problem here.
Please upvote 👍 this issue if you are interested in it.
The text was updated successfully, but these errors were encountered: