Skip to content
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

move IValueDescriptor to System.CommandLine.NamingConventionBinder #2124

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

adamsitnik
Copy link
Member

It was way easier than I expected, mostly because at the end of the day the crucial part of the code was casting IValueDescriptor to Argument and Symbol:

var specificDescriptor = ValueDescriptor;
switch (specificDescriptor)
{
case Option option:
var optionResult = bindingContext?.ParseResult.FindResultFor(option);
if (optionResult is not null)
{
boundValue = optionResult.GetValueOrDefault<object>();
return true;
}
break;
case Argument argument:
var argumentResult = bindingContext?.ParseResult.FindResultFor(argument);
if (argumentResult is not null)
{
boundValue = argumentResult.GetValueOrDefault<object>();
return true;
}
break;
}

fixes #1917
fixes #1918

@adamsitnik adamsitnik merged commit dcbd1c5 into dotnet:main Mar 29, 2023
@adamsitnik adamsitnik deleted the IValueDescriptor branch March 29, 2023 15:01
@@ -12,7 +12,7 @@ namespace System.CommandLine
/// <summary>
/// A symbol defining a named parameter and a value for that parameter.
/// </summary>
public abstract class Option : Symbol, IValueDescriptor
public abstract class Option : Symbol
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsitnik, do you have any recommendation what should be used here: https://github.com/dotnet/runtime/blob/f8bd9d29be973e0a9b82b87b9e50a0d3a77d1697/src/coreclr/tools/Common/CommandLineHelpers.cs#L187-L189? It (--make-repro) is a special facility where we create a repro package with command+arguments+artifacts (as zip). In there, we are checking if Option's value is default (set by default factory). Note this is not a hot path so using reflection is an option as a last resort. (to get by the compile error for now, I used reinterpretation style cast: (IValueDescriptor)(object)option 😅)

ps - I am in the middle of grand upgrade in runtime and jitutils repos. There is no rush, I'm gradually rebasing and picking up latest daily versions as I go further in my branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants