From 74b9cf9c39c689dab7848fbbdd670fdad75d9511 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 10:43:02 -0400 Subject: [PATCH 1/9] Implemented CalculatedValue --- OpenQuestions.md | 29 ++- ...System.CommandLine.Subsystems.Tests.csproj | 2 +- ...ubsystemTests.cs => ValueProviderTests.cs} | 77 +++----- .../ValueSourceTests.cs | 178 +++++++++-------- .../CalculatedValue.cs | 69 +++++++ .../PipelineResult.cs | 2 +- .../Annotations/ValueAnnotations.cs | 5 + .../System.CommandLine.Subsystems.csproj | 2 + .../ValueAnnotationExtensions.cs | 184 +----------------- .../ValueProvider.cs | 27 +-- src/System.CommandLine/CliCommand.cs | 17 +- 11 files changed, 263 insertions(+), 329 deletions(-) rename src/System.CommandLine.Subsystems.Tests/{ValueSubsystemTests.cs => ValueProviderTests.cs} (56%) create mode 100644 src/System.CommandLine.Subsystems/CalculatedValue.cs diff --git a/OpenQuestions.md b/OpenQuestions.md index 9dd1770bd..d80ad47c9 100644 --- a/OpenQuestions.md +++ b/OpenQuestions.md @@ -42,7 +42,7 @@ The first probably means we pass around `PipelineResult`. The second means that We started with `Validators` and then added the IValidator interface to allow conditions to do validation because they have the strong type. Checking for this first also avoids a dictionary lookup. -Our default validations will be on the Condition for the shortcut. Users can offer alternatives by creaing custom validators. The dictionary for custom validators will be lazy, and lookups will be pay for play when the user has custom validators. (This is not yet implemented.) +Our default validations will be on the Condition for the shortcut. Users can offer alternatives by creating custom validators. The dictionary for custom validators will be lazy, and lookups will be pay for play when the user has custom validators. (This is not yet implemented.) When present, custom validators have precedence. There is no cost when they are not present. @@ -56,4 +56,29 @@ Suggestion: Use internal constructors and leave conditions public ## Should `ValueCondition` be called `Condition`? -They may apply to commands. \ No newline at end of file +They may apply to commands. + +## Can we remove the "Annotations" in xxxxAnnotationExtensions + +We have other extensions, such as `AddCalculation`. Where should it go? + +They may shift to extension types in the future. + +It's a long in Solution Explorer + +## Calculated value design + +My first direction on the calculated value design was to derive from CliSymbol and treat them similarly to any other CliSymbol. This results in a technical challenge in the way the `Add` method works for CliSymbols - specifically it does not allow adding anything except options and arguments and the design results in infinite recursion if the exception is ignored. While we might be able to finesse this, it indicates just how thing the ice is if we try to "trick" things in the core parser layer. + +Instead calculated values are a new thing. They can contribute symbols when asked - their internal components can be expressed as symbols for help, for example. However, they are not a CliSymbol and for all uses must be separately treated. + +They are held on commands via annotations. Calculated values that should be are not logically located on a symbol should be on the root command. + +This will use collection annotations when they are available. For now they are List. + +We have a naming challenge that may indicate an underlying need to refactor: + +- ValueSource: Knows how to get data from disparate sources - constants, other symbols, environment variables. +- Calculation: Parameter/property on ValueSources allowing them to be relative to their source +- CalculatedValue (possibly CliCalculatedValue): A new thing that can be declared by the CliAuthor for late interpretation and type conversions. +- ValueCondition, ValueSymbol and other places where "Value" allows unification of Option and Argument (and is very, very helpful for that) \ No newline at end of file diff --git a/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj b/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj index ba321497f..e7f77c86c 100644 --- a/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj +++ b/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj @@ -34,7 +34,7 @@ - + diff --git a/src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs similarity index 56% rename from src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs rename to src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs index 9fb7573a9..664515f60 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueSubsystemTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs @@ -4,12 +4,13 @@ using FluentAssertions; using System.CommandLine.Directives; using System.CommandLine.Parsing; +using System.Runtime.Serialization; using Xunit; using static System.CommandLine.Subsystems.Tests.TestData; namespace System.CommandLine.Subsystems.Tests; -public class ValueSubsystemTests +public class ValueProviderTests { [Fact] public void Values_that_are_entered_are_retrieved() @@ -59,68 +60,42 @@ public void Values_that_are_not_entered_are_type_default_with_no_default_values( guidOptionValue.Should().Be(Guid.Empty); } - // TODO: Add various default value tests - - /* Hold these tests until we determine if ValueSubsystem is replaceable - [Fact] - public void ValueSubsystem_returns_values_that_are_entered() - { - var consoleHack = new ConsoleHack().RedirectToBuffer(true); - CliOption option = new CliOption("--intValue"); - CliRootCommand rootCommand = [ - new CliCommand("x") - { - option - }]; - var configuration = new CliConfiguration(rootCommand); - var pipeline = Pipeline.CreateEmpty(); - pipeline.Value = new ValueSubsystem(); - const int expected = 42; - var input = $"x --intValue {expected}"; - - var parseResult = pipeline.Parse(configuration, input); // assigned for debugging - pipeline.Execute(configuration, input, consoleHack); - - pipeline.Value.GetValue(option).Should().Be(expected); - } - [Fact] - public void ValueSubsystem_returns_default_value_when_no_value_is_entered() + public void Default_value_is_used_when_user_did_not_enter_it() { - var consoleHack = new ConsoleHack().RedirectToBuffer(true); - CliOption option = new CliOption("--intValue"); - CliRootCommand rootCommand = [option]; + var intOption = new CliOption("--intOption"); + intOption.SetDefault(42); + var rootCommand = new CliRootCommand { intOption }; var configuration = new CliConfiguration(rootCommand); - var pipeline = Pipeline.CreateEmpty(); - pipeline.Value = new ValueSubsystem(); - option.SetDefaultValue(43); - const int expected = 43; - var input = $""; + var pipeline = Pipeline.Create(); + var input = ""; - pipeline.Execute(configuration, input, consoleHack); + var parseResult = CliParser.Parse(rootCommand, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); - pipeline.Value.GetValue(option).Should().Be(expected); + pipelineResult.Should().NotBeNull(); + var intOptionValue = pipelineResult.GetValue(intOption); + intOptionValue.Should().Be(42); } - [Fact] - public void ValueSubsystem_returns_calculated_default_value_when_no_value_is_entered() + public void Can_retrieve_calculated_value_() { - var consoleHack = new ConsoleHack().RedirectToBuffer(true); - CliOption option = new CliOption("--intValue"); - CliRootCommand rootCommand = [option]; + // TODO: This is problematic because we probably can't and don't want to add new types + // of ValueSymbols. We need a different way to add these, which means we need to work + // out where they live (probably as annotations on the root command or a special symbol + // type) and how we add them (probably an extension method). + var calcSymbol = new CalculatedValue("calcValue", 42); + var rootCommand = new CliRootCommand { calcSymbol }; var configuration = new CliConfiguration(rootCommand); - var pipeline = Pipeline.CreateEmpty(); - pipeline.Value = new ValueSubsystem(); - var x = 42; - option.SetDefaultValueCalculation(() => x + 2); - const int expected = 44; + var pipeline = Pipeline.Create(); var input = ""; - var parseResult = pipeline.Parse(configuration, input); // assigned for debugging - pipeline.Execute(configuration, input, consoleHack); + var parseResult = CliParser.Parse(rootCommand, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); - pipeline.Value.GetValue(option).Should().Be(expected); + pipelineResult.Should().NotBeNull(); + var intOptionValue = pipelineResult.GetValue(calcSymbol); + intOptionValue.Should().Be(42); } - */ } diff --git a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs index 68a482b6b..3b1b7427a 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs @@ -2,9 +2,11 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; +using Microsoft.VisualBasic.FileIO; using System.CommandLine.Parsing; using System.CommandLine.ValueSources; using Xunit; +using static System.Net.Mime.MediaTypeNames; namespace System.CommandLine.Subsystems.Tests; @@ -26,13 +28,12 @@ public void SimpleValueSource_with_set_value_retrieved() { var valueSource = new SimpleValueSource(42); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -40,13 +41,12 @@ public void SimpleValueSource_with_converted_value_retrieved() { ValueSource valueSource = 42; - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -54,13 +54,13 @@ public void SimpleValueSource_created_via_extension_value_retrieved() { var valueSource = ValueSource.Create(42); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); + } [Fact] @@ -68,13 +68,12 @@ public void CalculatedValueSource_produces_value() { var valueSource = new CalculatedValueSource(() => (true, 42)); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -84,13 +83,12 @@ public void CalculatedValueSource_implicitly_converted_produces_value() // ValueSource valueSource2 = (() => 42); ValueSource valueSource = (ValueSource)(() => (true, 42)); ; - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -98,13 +96,12 @@ public void CalculatedValueSource_from_extension_produces_value() { var valueSource = ValueSource.Create(() => (true, 42)); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -113,14 +110,13 @@ public void RelativeToSymbolValueSource_produces_value_that_was_set() var option = new CliOption("-a"); var valueSource = new RelativeToSymbolValueSource(option); - if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); - + var found = valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); + } [Fact] @@ -129,13 +125,12 @@ public void RelativeToSymbolValueSource_implicitly_converted_produces_value_that var option = new CliOption("-a"); ValueSource valueSource = option; - if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -144,13 +139,12 @@ public void RelativeToSymbolValueSource_from_extension_produces_value_that_was_s var option = new CliOption("-a"); var valueSource = new RelativeToSymbolValueSource(option); - if (valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value)) - { - value.Should() - .Be(42); - return; - } - Assert.Fail("Typed value not retrieved"); + var found = valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); } [Fact] @@ -160,16 +154,14 @@ public void RelativeToEnvironmentVariableValueSource_produces_value_that_was_set var valueSource = new RelativeToEnvironmentVariableValueSource(envName); Environment.SetEnvironmentVariable(envName, "42"); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); Environment.SetEnvironmentVariable(envName, null); - Assert.Fail("Typed value not retrieved"); - } + found.Should() + .BeTrue(); + value.Should() + .Be(42); + } [Fact] public void RelativeToEnvironmentVariableValueSource_from_extension_produces_value_that_was_set() @@ -178,13 +170,41 @@ public void RelativeToEnvironmentVariableValueSource_from_extension_produces_val var valueSource = ValueSource.CreateFromEnvironmentVariable(envName); Environment.SetEnvironmentVariable(envName, "42"); - if (valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value)) - { - value.Should() - .Be(42); - return; - } + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); Environment.SetEnvironmentVariable(envName, null); - Assert.Fail("Typed value not retrieved"); + + found.Should() + .BeTrue(); + value.Should() + .Be(42); + } + + + [Fact] + public void RelativeToSymbolValueSource_false_if_other_symbol_missing() + { + var option = new CliOption("-a"); + var valueSource = new RelativeToSymbolValueSource(option); + + var found = valueSource.TryGetTypedValue(EmptyPipelineResult("", option), out var value); + found.Should() + .BeFalse(); + value.Should() + .Be(default); } + + [Fact] + public void RelativeToEnvironmentVariable_false_if_environment_variable_missing() + { + var envName = "SYSTEM_COMMANDLINE_TESTING"; + var valueSource = new RelativeToEnvironmentVariableValueSource(envName); + + var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); + + found.Should() + .BeFalse(); + value.Should() + .Be(default); + } + } diff --git a/src/System.CommandLine.Subsystems/CalculatedValue.cs b/src/System.CommandLine.Subsystems/CalculatedValue.cs new file mode 100644 index 000000000..371f45739 --- /dev/null +++ b/src/System.CommandLine.Subsystems/CalculatedValue.cs @@ -0,0 +1,69 @@ +using System.CommandLine.ValueSources; + +namespace System.CommandLine; + +/// +/// CalculatedValueSymbol lets ValueSource contribute to the data space of the application +/// with many standard features such as default values and the ability to participate in help. +/// +/// +/// Known scenarios: +/// +/// `dotnet nuget why`: where for historic reasons the first argument is optional and the second is required. +/// Completions: where the number of items to be displayed may come from an environment variable or a config file. +/// Compound types: where multiple value symbols contribute to the creation of a type, like -x and -y creating a point. +/// Reusing a value, for example to avoid multiple reads of a file +/// +/// CalculatedValueSymbol's should be available to subsystems, including invocation and conditions. +/// +public abstract class CalculatedValue : CliValueSymbol +{ + protected CalculatedValue(string name, ValueSource valueSource) + :base(name) + { + //Name = name; + ValueSource = valueSource; + } + + public static CalculatedValue CreateCalculatedValue(string name, ValueSource valueSource) + => new(name, valueSource); + + ///// + ///// Gets the name of the symbol. + ///// + //public string Name { get; } + + ///// + ///// Gets or sets the that the argument's parsed tokens will be converted to. + ///// + //public abstract Type ValueType { get; } + + public IEnumerable? AppearAs { get; set; } + + public ValueSource ValueSource { get; } + + internal bool TryGetValue(PipelineResult pipelineResult, out T? calculatedValue) + { + if (ValueSource.TryGetValue(pipelineResult, out object? objectValue)) + { + calculatedValue = (T?)objectValue; + return true; + } + calculatedValue = default; + return false; + } +} + +/// +/// CalculatedValueSymbol lets ValueSource contribute to the data space of the application +/// with many standard features such as default values and the ability to participate in help. +/// +/// The value type of the symbol. +public class CalculatedValue : CalculatedValue +{ + internal CalculatedValue(string name, ValueSource valueSource) + : base(name, valueSource) + { } + + public override Type ValueType { get; } = typeof(T); +} diff --git a/src/System.CommandLine.Subsystems/PipelineResult.cs b/src/System.CommandLine.Subsystems/PipelineResult.cs index 55b5f4963..9817be1ec 100644 --- a/src/System.CommandLine.Subsystems/PipelineResult.cs +++ b/src/System.CommandLine.Subsystems/PipelineResult.cs @@ -9,7 +9,7 @@ public class PipelineResult { // TODO: Try to build workflow so it is illegal to create this without a ParseResult private readonly List errors = []; - private ValueProvider valueProvider { get; } + private readonly ValueProvider valueProvider; public PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeline, ConsoleHack? consoleHack = null) { diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs index 7695c363e..e610d9bf8 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs @@ -36,4 +36,9 @@ public static class ValueAnnotations /// /// public static AnnotationId DefaultValueCalculation { get; } = new(Prefix, nameof(DefaultValueCalculation)); + + /// + /// + /// + public static AnnotationId CalculatedValues { get; } = new (Prefix, nameof(CalculatedValues)); } diff --git a/src/System.CommandLine.Subsystems/System.CommandLine.Subsystems.csproj b/src/System.CommandLine.Subsystems/System.CommandLine.Subsystems.csproj index 2f5b0e4d9..c3185ba92 100644 --- a/src/System.CommandLine.Subsystems/System.CommandLine.Subsystems.csproj +++ b/src/System.CommandLine.Subsystems/System.CommandLine.Subsystems.csproj @@ -6,6 +6,8 @@ enable System.CommandLine true + + diff --git a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs index 676bcc9b7..bb4d69269 100644 --- a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs @@ -21,8 +21,8 @@ public static class ValueAnnotationExtensions /// which calculates the actual default value, based on the default value annotation and default value calculation, /// whether directly stored on the symbol or from the subsystem's . /// - public static bool TryGetDefaultValueSource(this CliValueSymbol valueSymbol, [NotNullWhen(true)] out ValueSource? defaultValueSource) - => valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValueSource, out defaultValueSource); + public static bool TryGetDefault(this CliValueSymbol valueSymbol, out ValueSource? defaultValueSource) + => valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out defaultValueSource); /// /// Sets the default value annotation on the @@ -30,185 +30,15 @@ public static bool TryGetDefaultValueSource(this CliValueSymbol valueSymbol, [No /// The type of the option value /// The option /// The default value for the option - public static void SetDefaultValueSource(this CliValueSymbol valueSymbol, ValueSource defaultValue) + public static void SetDefault(this CliValueSymbol valueSymbol, ValueSource defaultValue) => valueSymbol.SetAnnotation(ValueAnnotations.DefaultValue, defaultValue); - - /// - /// Sets the default value annotation on the - /// - /// The type of the option value - /// The option - /// The default value for the option - /// The , to enable fluent construction of symbols with annotations. - public static CliOption WithDefaultValue(this CliOption option, TValue defaultValue) - { - option.SetDefaultValue(defaultValue); - return option; - } - - /// - /// Sets the default value annotation on the - /// - /// The type of the option value - /// The option - /// The default value for the option - public static void SetDefaultValue(this CliOption option, TValue defaultValue) - { - option.SetAnnotation(ValueAnnotations.DefaultValue, defaultValue); - } - - /// - /// Get the default value annotation for the - /// - /// The type of the option value - /// The option - /// The option's default value annotation if any, otherwise - /// - /// This is intended to be called by CLI authors. Subsystems should instead call , - /// which calculates the actual default value, based on the default value annotation and default value calculation, - /// whether directly stored on the symbol or from the subsystem's . - /// - public static TValue? GetDefaultValueAnnotation(this CliOption option) - { - if (option.TryGetAnnotation(ValueAnnotations.DefaultValue, out TValue? defaultValue)) - { - return defaultValue; - } - return default; - } - - /// - /// Sets the default value annotation on the - /// - /// The type of the argument value - /// The argument - /// The default value for the argument - /// The , to enable fluent construction of symbols with annotations. - public static CliArgument WithDefaultValue(this CliArgument argument, TValue defaultValue) - { - argument.SetDefaultValue(defaultValue); - return argument; - } - - /// - /// Sets the default value annotation on the - /// - /// The type of the argument value - /// The argument - /// The default value for the argument - /// The , to enable fluent construction of symbols with annotations. - public static void SetDefaultValue(this CliArgument argument, TValue defaultValue) - { - argument.SetAnnotation(ValueAnnotations.DefaultValue, defaultValue); - } - - /// - /// Get the default value annotation for the - /// - /// The type of the argument value - /// The argument - /// The argument's default value annotation if any, otherwise - /// - /// This is intended to be called by CLI authors. Subsystems should instead call , - /// which calculates the actual default value, based on the default value annotation and default value calculation, - /// whether directly stored on the symbol or from the subsystem's . - /// - public static TValue? GetDefaultValueAnnotation(this CliArgument argument) - { - if (argument.TryGetAnnotation(ValueAnnotations.DefaultValue, out TValue? defaultValue)) - { - return (TValue?)defaultValue; - } - return default; - } - - /// - /// Sets the default value calculation for the - /// - /// The type of the option value - /// The option - /// The default value calculation for the option - /// The , to enable fluent construction of symbols with annotations. - public static CliOption WithDefaultValueCalculation(this CliOption option, Func defaultValueCalculation) - { - option.SetDefaultValueCalculation(defaultValueCalculation); - return option; - } - - /// - /// Sets the default value calculation for the - /// - /// The type of the option value - /// The option - /// The default value calculation for the option - public static void SetDefaultValueCalculation(this CliOption option, Func defaultValueCalculation) - { - option.SetAnnotation(ValueAnnotations.DefaultValueCalculation, defaultValueCalculation); - } - - /// - /// Get the default value calculation for the - /// - /// The type of the option value - /// The option - /// The option's default value calculation if any, otherwise - /// - /// This is intended to be called by CLI authors. Subsystems should instead call , - /// which calculates the actual default value, based on the default value annotation and default value calculation, - /// whether directly stored on the symbol or from the subsystem's . - /// - public static Func? GetDefaultValueCalculation(this CliOption option) - { - if (option.TryGetAnnotation(ValueAnnotations.DefaultValueCalculation, out Func? defaultValueCalculation)) - { - return defaultValueCalculation; - } - return default; - } - - /// - /// Sets the default value calculation for the - /// - /// The type of the argument value - /// The argument - /// The default value calculation for the argument - /// The , to enable fluent construction of symbols with annotations. - public static CliArgument WithDefaultValueCalculation(this CliArgument argument, Func defaultValueCalculation) - { - argument.SetDefaultValueCalculation(defaultValueCalculation); - return argument; - } - - /// - /// Sets the default value calculation for the - /// - /// The type of the argument value - /// The argument - /// The default value calculation for the argument - /// The , to enable fluent construction of symbols with annotations. - public static void SetDefaultValueCalculation(this CliArgument argument, Func defaultValueCalculation) - { - argument.SetAnnotation(ValueAnnotations.DefaultValueCalculation, defaultValueCalculation); - } - - /// - /// Get the default value calculation for the - /// - /// The type of the argument value - /// The argument - /// The argument's default value calculation if any, otherwise - /// - /// This is intended to be called by CLI authors. Subsystems should instead call , - /// which calculates the actual default value, based on the default value annotation and default value calculation, - /// whether directly stored on the symbol or from the subsystem's . - /// - public static Func? GetDefaultValueCalculation(this CliArgument argument) + public static void AddCalculatedValue(this CliCommand command, CalculatedValue calculatedValue) { - if (argument.TryGetAnnotation(ValueAnnotations.DefaultValueCalculation, out Func? defaultValueCalculation)) + if (!command.TryGetAnnotation>(ValueAnnotations.CalculatedValues, out var currentList)) { - return defaultValueCalculation; + currentList = new List(); } - return default; + currentList.Add(calculatedValue); } } diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index f51a1edb6..0a0f7c638 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -40,15 +40,22 @@ private bool TryGetValue(CliSymbol symbol, out T? value) { // TODO: Add guard to prevent reentrancy for the same symbol via RelativeToSymbol of custom ValueSource var _ = valueSymbol ?? throw new ArgumentNullException(nameof(valueSymbol)); - if (TryGetValue(valueSymbol, out var value)) + + if (TryGetFromCache(valueSymbol, out var cachedValue)) { - return value; + return cachedValue; + } + if (valueSymbol is CalculatedValue calculatedValueSymbol + && calculatedValueSymbol.TryGetValue(pipelineResult, out T? calculatedValue)) + { + return UseValue(valueSymbol, calculatedValue); } - if (pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) + if (valueSymbol is not CalculatedValue + && pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) { return UseValue(valueSymbol, valueResult.GetValue()); } - if (valueSymbol.TryGetDefaultValueSource(out ValueSource? defaultValueSource)) + if (valueSymbol.TryGetDefault(out ValueSource? defaultValueSource)) { if (defaultValueSource is not ValueSource typedDefaultValueSource) { @@ -61,19 +68,13 @@ private bool TryGetValue(CliSymbol symbol, out T? value) } return UseValue(valueSymbol, default(T)); + bool TryGetFromCache(CliValueSymbol valueSymbol, out T? cachedValue) + => TryGetValue(valueSymbol, out cachedValue); + TValue UseValue(CliSymbol symbol, TValue value) { SetValue(symbol, value); return value; } } - - private static T? CalculatedDefault(CliValueSymbol valueSymbol, Func defaultValueCalculation) - { - var objectValue = defaultValueCalculation(); - var value = objectValue is null - ? default - : (T)objectValue; - return value; - } } diff --git a/src/System.CommandLine/CliCommand.cs b/src/System.CommandLine/CliCommand.cs index 8adccb5d9..27a28858d 100644 --- a/src/System.CommandLine/CliCommand.cs +++ b/src/System.CommandLine/CliCommand.cs @@ -27,8 +27,9 @@ public class CliCommand : CliSymbol, IEnumerable internal AliasSet? _aliases; private ChildSymbolList? _arguments; private ChildSymbolList? _options; + private ChildSymbolList? _otherValueSymbols; private ChildSymbolList? _subcommands; -// TODO: validators + // TODO: validators /* private List>? _validators; @@ -78,6 +79,8 @@ public IEnumerable Children // TODO: Consider value of lazy here. It sets up a desire to use awkward approach (HasOptions) for a perf win. Applies to Options and Subcommands also. public IList Options => _options ??= new (this); + public IList OtherSymbols => _otherValueSymbols ??= new(this); + internal bool HasOptions => _options?.Count > 0; /// @@ -86,7 +89,7 @@ public IEnumerable Children public IList Subcommands => _subcommands ??= new(this); internal bool HasSubcommands => _subcommands is not null && _subcommands.Count > 0; -/* + /* /// /// Validators to the command. Validators can be used /// to create custom validation logic. @@ -195,7 +198,7 @@ public void SetAction(Func> action) // Hide from IntelliSense as it's only to support initializing via C# collection expression // More specific efficient overloads are available for all supported symbol types. - [DebuggerStepThrough] + //[DebuggerStepThrough] [EditorBrowsable(EditorBrowsableState.Never)] public void Add(CliSymbol symbol) { @@ -211,9 +214,13 @@ public void Add(CliSymbol symbol) { Add(command); } - else + else if (symbol is CliValueSymbol valueSymbol) + { + OtherSymbols.Add(valueSymbol); + } + else { -// TODO: add a localized message here + // TODO: add a localized message here throw new ArgumentException(null, nameof(symbol)); } } From ac619180afba18db4db70b669c31f6dcc000dcf5 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 11:24:19 -0400 Subject: [PATCH 2/9] Updated ValueProvider for correct TryGet handling --- .../ValueSourceTests.cs | 2 +- .../PipelineResult.cs | 8 +++ .../ValueProvider.cs | 62 +++++++++++-------- .../RelativeToSymbolValueSource.cs | 24 +++---- 4 files changed, 58 insertions(+), 38 deletions(-) diff --git a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs index 3b1b7427a..87773b4b5 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs @@ -181,7 +181,7 @@ public void RelativeToEnvironmentVariableValueSource_from_extension_produces_val [Fact] - public void RelativeToSymbolValueSource_false_if_other_symbol_missing() + public void RelativeToSymbolValueSource_false_if_other_symbol_has_no_default_and_is_missing() { var option = new CliOption("-a"); var valueSource = new RelativeToSymbolValueSource(option); diff --git a/src/System.CommandLine.Subsystems/PipelineResult.cs b/src/System.CommandLine.Subsystems/PipelineResult.cs index 9817be1ec..534244eb0 100644 --- a/src/System.CommandLine.Subsystems/PipelineResult.cs +++ b/src/System.CommandLine.Subsystems/PipelineResult.cs @@ -36,6 +36,14 @@ public PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeli public object? GetValue(CliValueSymbol option) => valueProvider.GetValue(option); + public bool TryGetValue(CliValueSymbol valueSymbol, out T? value) + => valueProvider.TryGetValue(valueSymbol, out value); + + public bool TryGetValue(CliValueSymbol option, out object? value) + => valueProvider.TryGetValue(option, out value); + + + public CliValueResult? GetValueResult(CliValueSymbol valueSymbol) => ParseResult.GetValueResult(valueSymbol); diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index 0a0f7c638..f6e33f127 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -2,12 +2,13 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.CommandLine.ValueSources; +using CacheItem = (bool found, object? value); namespace System.CommandLine; internal class ValueProvider { - private Dictionary cachedValues = []; + private Dictionary cachedValues = []; private PipelineResult pipelineResult; public ValueProvider(PipelineResult pipelineResult) @@ -15,66 +16,75 @@ public ValueProvider(PipelineResult pipelineResult) this.pipelineResult = pipelineResult; } - private void SetValue(CliSymbol symbol, object? value) + private void SetValue(CliSymbol symbol, object? value, bool found) { - cachedValues[symbol] = value; + cachedValues[symbol] = (found, value); } - private bool TryGetValue(CliSymbol symbol, out T? value) + private bool TryGetFromCache(CliSymbol symbol, out T? value) { - if (cachedValues.TryGetValue(symbol, out var objectValue)) + if (cachedValues.TryGetValue(symbol, out var t)) { - value = objectValue is null - ? default - : (T)objectValue; - return true; + if (t.found) + { + value = (T?)t.value; + return true; + } } value = default; return false; } public T? GetValue(CliValueSymbol valueSymbol) - => GetValueInternal(valueSymbol); + { + if (TryGetValueInternal(valueSymbol, out var value)) + { + return value; + } + return default; + } + + public bool TryGetValue(CliValueSymbol valueSymbol, out T? value) + => TryGetValueInternal(valueSymbol, out value); - private T? GetValueInternal(CliValueSymbol valueSymbol) + private bool TryGetValueInternal(CliValueSymbol valueSymbol, out T? value) { // TODO: Add guard to prevent reentrancy for the same symbol via RelativeToSymbol of custom ValueSource var _ = valueSymbol ?? throw new ArgumentNullException(nameof(valueSymbol)); - if (TryGetFromCache(valueSymbol, out var cachedValue)) + if (TryGetFromCache(valueSymbol, out value)) { - return cachedValue; + return true; } if (valueSymbol is CalculatedValue calculatedValueSymbol - && calculatedValueSymbol.TryGetValue(pipelineResult, out T? calculatedValue)) + && calculatedValueSymbol.TryGetValue(pipelineResult, out value)) { - return UseValue(valueSymbol, calculatedValue); + return CacheAndReturnSuccess(valueSymbol, value, true); } if (valueSymbol is not CalculatedValue && pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) { - return UseValue(valueSymbol, valueResult.GetValue()); + value = valueResult.GetValue(); + return CacheAndReturnSuccess(valueSymbol, value, true); } if (valueSymbol.TryGetDefault(out ValueSource? defaultValueSource)) { if (defaultValueSource is not ValueSource typedDefaultValueSource) { - throw new InvalidOperationException("Unexpected ValueSource type"); + throw new InvalidOperationException("Unexpected ValueSource type for default value."); } - if (typedDefaultValueSource.TryGetTypedValue(pipelineResult, out T? defaultValue)) + if (typedDefaultValueSource.TryGetTypedValue(pipelineResult, out value)) { - return UseValue(valueSymbol, defaultValue); + return CacheAndReturnSuccess(valueSymbol, value, true); } } - return UseValue(valueSymbol, default(T)); - - bool TryGetFromCache(CliValueSymbol valueSymbol, out T? cachedValue) - => TryGetValue(valueSymbol, out cachedValue); + // TODO: Determine if we should cache default. If so, additional design is needed to avoid first hit returning false, and remainder returning true + return CacheAndReturnSuccess(valueSymbol, default, false); - TValue UseValue(CliSymbol symbol, TValue value) + bool CacheAndReturnSuccess(CliValueSymbol valueSymbol, T? valueToCache, bool valueFound) { - SetValue(symbol, value); - return value; + SetValue(valueSymbol, valueToCache, valueFound); + return valueFound; } } } diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs index a53e13c57..14654426e 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs @@ -40,19 +40,21 @@ public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? valu return false; } - var otherSymbolValue = pipelineResult.GetValue(OtherSymbol); - - if (Calculation is null) - { - value = otherSymbolValue; - return true; - } - (var success, var newValue) = Calculation(otherSymbolValue); - if (success) + if (pipelineResult.TryGetValue(OtherSymbol, out var otherSymbolValue)) { - value = newValue; - return true; + if (Calculation is null) + { + value = otherSymbolValue; + return true; + } + (var success, var newValue) = Calculation(otherSymbolValue); + if (success) + { + value = newValue; + return true; + } } + value = default; return false; } From 5ee18e36696ab5354a30655aa6ab5c14ccd29a20 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 12:26:48 -0400 Subject: [PATCH 3/9] Added reentrancy test --- .../ValueProviderTests.cs | 23 ++++++- .../ValueProvider.cs | 65 ++++++++++++------- 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs index 664515f60..0a0e47dfc 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs @@ -61,7 +61,7 @@ public void Values_that_are_not_entered_are_type_default_with_no_default_values( } [Fact] - public void Default_value_is_used_when_user_did_not_enter_it() + public void Default_value_is_used_when_user_did_not_enter_a_value() { var intOption = new CliOption("--intOption"); intOption.SetDefault(42); @@ -98,4 +98,25 @@ public void Can_retrieve_calculated_value_() var intOptionValue = pipelineResult.GetValue(calcSymbol); intOptionValue.Should().Be(42); } + + [Fact] + public void Circular_default_value_dependency_throw() + { + var opt1 = new CliOption("opt1"); + var opt2 = new CliOption("opt2"); + var opt3 = new CliOption("opt3"); + opt1.SetDefault(opt2); + opt2.SetDefault(opt3); + opt3.SetDefault(opt1); + var rootCommand = new CliRootCommand { opt1, opt2, opt3 }; + var configuration = new CliConfiguration(rootCommand); + var pipeline = Pipeline.Create(); + var input = ""; + + var parseResult = CliParser.Parse(rootCommand, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); + + pipelineResult.Should().NotBeNull(); + Assert.Throws(() => _ = pipelineResult.GetValue(opt1)); + } } diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index f6e33f127..22e69dba3 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -2,12 +2,18 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.CommandLine.ValueSources; -using CacheItem = (bool found, object? value); namespace System.CommandLine; internal class ValueProvider { + private struct CacheItem + { + internal object? Value; + internal bool WasFound; + internal bool IsCalculating; + } + private Dictionary cachedValues = []; private PipelineResult pipelineResult; @@ -16,23 +22,23 @@ public ValueProvider(PipelineResult pipelineResult) this.pipelineResult = pipelineResult; } - private void SetValue(CliSymbol symbol, object? value, bool found) + private void SetCachedValue(CliSymbol symbol, object? value, bool found) { - cachedValues[symbol] = (found, value); + // TODO: MHutch: Feel free to optimize this and SetIsCalculating. We need the struct or a tuple here for "WasFound" which turns out we need. + cachedValues[symbol] = new CacheItem() + { + Value = value, + WasFound = found, + IsCalculating = false + }; } - private bool TryGetFromCache(CliSymbol symbol, out T? value) + private void SetIsCalculating(CliSymbol symbol) { - if (cachedValues.TryGetValue(symbol, out var t)) + cachedValues[symbol] = new CacheItem() { - if (t.found) - { - value = (T?)t.value; - return true; - } - } - value = default; - return false; + IsCalculating = true + }; } public T? GetValue(CliValueSymbol valueSymbol) @@ -49,23 +55,37 @@ public bool TryGetValue(CliValueSymbol valueSymbol, out T? value) private bool TryGetValueInternal(CliValueSymbol valueSymbol, out T? value) { - // TODO: Add guard to prevent reentrancy for the same symbol via RelativeToSymbol of custom ValueSource var _ = valueSymbol ?? throw new ArgumentNullException(nameof(valueSymbol)); - if (TryGetFromCache(valueSymbol, out value)) + if (cachedValues.TryGetValue(valueSymbol, out var cacheItem)) { - return true; + if (cacheItem.IsCalculating) + { + value = default; + // Guard against reentrancy. Placed here so we catch if a CalculatedValue or calculation causes reentrancy + throw new InvalidOperationException("Circular value source dependency."); + } + if (cacheItem.WasFound) + { + value = (T?)cacheItem.Value; + return true; + } } + // !!! CRITICAL: All returns from this method should set the cache value to clear this pseudo-lock (use CacheAndReturn) + // TODO: Using the cache would only work if we gave up the cache returns being strongly typed. + // Consider instead a + SetIsCalculating(valueSymbol); + if (valueSymbol is CalculatedValue calculatedValueSymbol && calculatedValueSymbol.TryGetValue(pipelineResult, out value)) { - return CacheAndReturnSuccess(valueSymbol, value, true); + return CacheAndReturn(valueSymbol, value, true); } if (valueSymbol is not CalculatedValue && pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) { value = valueResult.GetValue(); - return CacheAndReturnSuccess(valueSymbol, value, true); + return CacheAndReturn(valueSymbol, value, true); } if (valueSymbol.TryGetDefault(out ValueSource? defaultValueSource)) { @@ -75,15 +95,16 @@ private bool TryGetValueInternal(CliValueSymbol valueSymbol, out T? value) } if (typedDefaultValueSource.TryGetTypedValue(pipelineResult, out value)) { - return CacheAndReturnSuccess(valueSymbol, value, true); + return CacheAndReturn(valueSymbol, value, true); } } // TODO: Determine if we should cache default. If so, additional design is needed to avoid first hit returning false, and remainder returning true - return CacheAndReturnSuccess(valueSymbol, default, false); + value = default; + return CacheAndReturn(valueSymbol, value, false); - bool CacheAndReturnSuccess(CliValueSymbol valueSymbol, T? valueToCache, bool valueFound) + bool CacheAndReturn(CliValueSymbol valueSymbol, T? valueToCache, bool valueFound) { - SetValue(valueSymbol, valueToCache, valueFound); + SetCachedValue(valueSymbol, valueToCache, valueFound); return valueFound; } } From a13bf785452c33741f2035e57e465e2e52e113de Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 15:14:13 -0400 Subject: [PATCH 4/9] Added `dotnet nuget why` test and fixed --- OpenQuestions.md | 14 +++- .../ValueProviderTests.cs | 80 +++++++++++++++++++ .../RelativeToSymbolValueSource.cs | 4 +- 3 files changed, 95 insertions(+), 3 deletions(-) diff --git a/OpenQuestions.md b/OpenQuestions.md index d80ad47c9..874897f8b 100644 --- a/OpenQuestions.md +++ b/OpenQuestions.md @@ -81,4 +81,16 @@ We have a naming challenge that may indicate an underlying need to refactor: - ValueSource: Knows how to get data from disparate sources - constants, other symbols, environment variables. - Calculation: Parameter/property on ValueSources allowing them to be relative to their source - CalculatedValue (possibly CliCalculatedValue): A new thing that can be declared by the CliAuthor for late interpretation and type conversions. -- ValueCondition, ValueSymbol and other places where "Value" allows unification of Option and Argument (and is very, very helpful for that) \ No newline at end of file +- ValueCondition, ValueSymbol and other places where "Value" allows unification of Option and Argument (and is very, very helpful for that) + +## Tests that do not have `InternalsVisibleTo` + +Currently all the code we have is in the `internal` scope within the logical layer. As a result, we are not testing how the libary works from the outside. We could take either of these two approaches or do something else, but I think we need to do something soon to validate our design: + +- Isolate the very small amount of code that actually needs IVT into an additional test assembly for each logical layer. + - This would result in two new projects and IVT would be removed from the current test project. + - The benefit of this approach is that the largest amount of code would be running without IVT. +- Create "example" projects: + - This would result in one or two new projects, two if the dependencies are those that you would use when accessing either the raw parsing behavior or subsystems. Current tests would remain under IVT. + - The benefit of this approach is that we will be creating functional tests in the form of "how do I do xxx". Since they are tests, they would not get out of date with the code base. + - Another potential benefit would be a bounded space where we could focus on how our APIs work in practice. \ No newline at end of file diff --git a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs index 0a0e47dfc..91ea44b5c 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs @@ -4,6 +4,7 @@ using FluentAssertions; using System.CommandLine.Directives; using System.CommandLine.Parsing; +using System.CommandLine.ValueSources; using System.Runtime.Serialization; using Xunit; using static System.CommandLine.Subsystems.Tests.TestData; @@ -119,4 +120,83 @@ public void Circular_default_value_dependency_throw() pipelineResult.Should().NotBeNull(); Assert.Throws(() => _ = pipelineResult.GetValue(opt1)); } + + [Fact] + public void dotnet_nuget_why_can_retrieve_project_and_package() + { + var opt1 = new CliOption("opt1"); + var opt2 = new CliOption("opt2"); + var opt3 = new CliOption("opt3"); + opt1.SetDefault(opt2); + opt2.SetDefault(opt3); + opt3.SetDefault(opt1); + var rootCommand = new CliRootCommand { opt1, opt2, opt3 }; + var configuration = new CliConfiguration(rootCommand); + var pipeline = Pipeline.Create(); + var input = ""; + + var parseResult = CliParser.Parse(rootCommand, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); + + pipelineResult.Should().NotBeNull(); + Assert.Throws(() => _ = pipelineResult.GetValue(opt1)); + } + + private (CliRootCommand root, CalculatedValue project, CalculatedValue package) CreateDotnetNugetWhyCli() + { + var whyArg = new CliArgument("whyArgs"); + whyArg.Arity = new ArgumentArity(1, 2); + var project = new CalculatedValue("project", ValueSource.Create(whyArg, ExtractProject)); + var package = new CalculatedValue("package", ValueSource.Create(whyArg, ExtractPackage)); + var whyCmd = new CliCommand("why") { whyArg }; + var nugetCmd = new CliCommand("nuget") { whyCmd }; + var dotnetCmd = new CliRootCommand() { nugetCmd }; + return (dotnetCmd, project, package); + + (bool success, string value) ExtractProject(object? input) + { + if (input is not string[] inputArray) + { + return (false, string.Empty); + } + return (true, + inputArray.Length == 1 + ? "" + : inputArray[0]); + } + + (bool success, string value) ExtractPackage(object? input) + { + if (input is not string[] inputArray) + { + return (false, string.Empty); + } + return (true, + inputArray.Length == 1 + ? inputArray[0] + : inputArray[1]); + } + } + + [Theory] + [InlineData("myProject.csproj myPackage", "myProject.csproj", "myPackage")] + [InlineData("myPackage", "", "myPackage")] + public void dotnet_nuget_why_can_retrieve_package_without_project(string args, string projectName, string packageName) + { + var (dotnetCmd, project, package) = CreateDotnetNugetWhyCli(); + var configuration = new CliConfiguration(dotnetCmd); + var pipeline = Pipeline.Create(); + var input = $"nuget why {args}"; + + var parseResult = CliParser.Parse(dotnetCmd, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); + + pipelineResult.Should().NotBeNull(); + pipelineResult.GetValue(project) + .Should() + .Be(projectName); + pipelineResult.GetValue(package) + .Should() + .Be(packageName); + } } diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs index 14654426e..bb238b3f1 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs @@ -40,11 +40,11 @@ public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? valu return false; } - if (pipelineResult.TryGetValue(OtherSymbol, out var otherSymbolValue)) + if (pipelineResult.TryGetValue(OtherSymbol, out var otherSymbolValue)) { if (Calculation is null) { - value = otherSymbolValue; + value = (T?)otherSymbolValue; return true; } (var success, var newValue) = Calculation(otherSymbolValue); From 72653452cfe07e4e4ea55c28b24dd995380024fb Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 17:25:11 -0400 Subject: [PATCH 5/9] Added support for multiple symbols and Point example --- .../ValueProviderTests.cs | 50 ++++++++++++++ .../RelativeToSymbolValueSource.cs | 3 +- .../RelativeToSymbolsValueSource.cs | 67 +++++++++++++++++++ .../ValueSources/ValueSource.cs | 9 ++- 4 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs diff --git a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs index 91ea44b5c..85c613f87 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs @@ -199,4 +199,54 @@ public void dotnet_nuget_why_can_retrieve_package_without_project(string args, s .Should() .Be(packageName); } + + private record struct Point2D(int X, int Y) + { + public static readonly Point2D Empty = new Point2D(0, 0); + } + + private (CliRootCommand root, CliValueSymbol point) CreatePointCli() + { + var xOpt = new CliOption("-x"); + var yOpt = new CliOption("-y"); + var point = new CalculatedValue("point", ValueSource.Create(MakePoint, otherSymbols: [xOpt, yOpt])); + var dotnetCmd = new CliRootCommand() { xOpt, yOpt }; + return (dotnetCmd, point); + + (bool success, Point2D point) MakePoint(IEnumerable input) + { + var inputInts = input.OfType().ToArray(); + if (inputInts.Length != 2) + { + return (false, Point2D.Empty); // since false is used, should return null, not empty point + } + return (true, + new Point2D(inputInts[0], inputInts[1])); + } + } + + [Theory] + [InlineData("-x 0 -y 0", "Point2D { X = 0, Y = 0 }")] + [InlineData("-x 1", "")] + [InlineData("-y 1", "")] + [InlineData("-x 1 -y 3", "Point2D { X = 1, Y = 3 }")] + [InlineData("-x -1 -y 2", "Point2D { X = -1, Y = 2 }")] + [InlineData("-x -2 -y -3", "Point2D { X = -2, Y = -3 }")] + public void Can_make_point_from_two_options(string input, string expected) + { + var (dotnetCmd, point) = CreatePointCli(); + var configuration = new CliConfiguration(dotnetCmd); + var pipeline = Pipeline.Create(); + + var parseResult = CliParser.Parse(dotnetCmd, input, configuration); + var pipelineResult = new PipelineResult(parseResult, input, pipeline); + + pipelineResult.Should().NotBeNull(); + var pointValue = pipelineResult.GetValue(point); + var output = $"{pointValue}"; + output + .Should() + .Be(expected); + } + } diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs index bb238b3f1..68621ef6b 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs @@ -14,10 +14,11 @@ namespace System.CommandLine.ValueSources; public sealed class RelativeToSymbolValueSource : ValueSource { + // TODO: API differences between this adn RelativeToSymbols are very annoying internal RelativeToSymbolValueSource( CliValueSymbol otherSymbol, - bool onlyUserEnteredValues = false, Func? calculation = null, + bool onlyUserEnteredValues = false, string? description = null) { OtherSymbol = otherSymbol; diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs new file mode 100644 index 000000000..c9a0cf02e --- /dev/null +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs @@ -0,0 +1,67 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace System.CommandLine.ValueSources; + +/// +/// that returns the value of the specified other symbol. +/// If the calculation delegate is supplied, the returned value of the calculation is returned. +/// +/// The type to be returned, which is almost always the type of the symbol the ValueSource will be used for. +/// The option or argument to return, with the calculation supplied if it is not null. +/// A delegate that returns the requested type. +/// The description of this value, used to clarify the intent of the values that appear in error messages. + // TODO: Do we want this to be an aggregate, such that you could build a type from other symbols, calcs and env variables. Ooo aahh +public sealed class RelativeToSymbolsValueSource + : ValueSource +{ + internal RelativeToSymbolsValueSource( + Func, (bool success, T? value)> calculation, + bool onlyUserEnteredValues = false, + string? description = null, + params CliValueSymbol[] otherSymbols) + { + OtherSymbols = otherSymbols; + OnlyUserEnteredValues = onlyUserEnteredValues; + Calculation = calculation; + Description = description; + } + + public override string? Description { get; } + public IEnumerable OtherSymbols { get; } + public bool OnlyUserEnteredValues { get; } + public Func, (bool success, T? value)> Calculation { get; } + + /// + public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value) + { + // TODO: How do we test for only user values. (no defaults) + //if (OnlyUserEnteredValues && pipelineResult.GetValueResult(OtherSymbol) is null) + //{ + // value = default; + // return false; + //} + + var otherSymbolValues = OtherSymbols.Select(GetOtherSymbolValues).ToArray(); + (var success, var newValue) = Calculation(otherSymbolValues); + if (success) + { + value = newValue; + return true; + } + + value = default; + return false; + + object? GetOtherSymbolValues(CliValueSymbol otherSymbol) + { + if (pipelineResult.TryGetValue(otherSymbol, out var otherSymbolValue)) + { + return otherSymbolValue; + } + // TODO: I suspect we will need more data here, such as whether it exists and whether user entered + return null; + } + } +} + diff --git a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs index 43098896e..ed2b81c3e 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs @@ -34,7 +34,14 @@ public static ValueSource Create(CliValueSymbol otherSymbol, Func? calculation = null, bool userEnteredValueOnly = false, string? description = null) - => new RelativeToSymbolValueSource(otherSymbol, userEnteredValueOnly, calculation, description); + => new RelativeToSymbolValueSource(otherSymbol, calculation, userEnteredValueOnly, description); + + public static ValueSource Create( + Func, (bool success, T? value)> calculation, + bool userEnteredValueOnly = false, + string? description = null, + params CliValueSymbol[] otherSymbols) + => new RelativeToSymbolsValueSource(calculation, userEnteredValueOnly, description, otherSymbols); public static ValueSource Create(ValueSource firstSource, ValueSource secondSource, From 8b36b4b6a06482fc7f316307786089a48dc89125 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sun, 1 Sep 2024 17:42:07 -0400 Subject: [PATCH 6/9] General cleanup --- .../CalculatedValue.cs | 25 +++++++++++-------- .../PipelineResult.cs | 3 --- .../ValueProvider.cs | 2 -- .../RelativeToSymbolsValueSource.cs | 16 ++++++++++++ 4 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/System.CommandLine.Subsystems/CalculatedValue.cs b/src/System.CommandLine.Subsystems/CalculatedValue.cs index 371f45739..cbaa61ca3 100644 --- a/src/System.CommandLine.Subsystems/CalculatedValue.cs +++ b/src/System.CommandLine.Subsystems/CalculatedValue.cs @@ -21,25 +21,27 @@ public abstract class CalculatedValue : CliValueSymbol protected CalculatedValue(string name, ValueSource valueSource) :base(name) { - //Name = name; ValueSource = valueSource; } + /// + /// Create a CalculatedValue. + /// + /// The type of the value that can be retrieved via this calculated value. + // TODO: Provide name lookup of CalculatedValues + /// The name of the calculated value + /// The ValueSource used to retrieve the value. If there are defaults/fallbacks, this will be an aggregate value source. + /// public static CalculatedValue CreateCalculatedValue(string name, ValueSource valueSource) => new(name, valueSource); - ///// - ///// Gets the name of the symbol. - ///// - //public string Name { get; } - - ///// - ///// Gets or sets the that the argument's parsed tokens will be converted to. - ///// - //public abstract Type ValueType { get; } - + // TODO: This feels backwards. Should probably appear on the string array used as the data source. public IEnumerable? AppearAs { get; set; } + /// + /// The ValueSource used to retrieve the value. If there are defaults/fallbacks, this will be an + /// aggregate value source. + /// public ValueSource ValueSource { get; } internal bool TryGetValue(PipelineResult pipelineResult, out T? calculatedValue) @@ -65,5 +67,6 @@ internal CalculatedValue(string name, ValueSource valueSource) : base(name, valueSource) { } + /// public override Type ValueType { get; } = typeof(T); } diff --git a/src/System.CommandLine.Subsystems/PipelineResult.cs b/src/System.CommandLine.Subsystems/PipelineResult.cs index 534244eb0..edd0e04ab 100644 --- a/src/System.CommandLine.Subsystems/PipelineResult.cs +++ b/src/System.CommandLine.Subsystems/PipelineResult.cs @@ -42,12 +42,9 @@ public bool TryGetValue(CliValueSymbol valueSymbol, out T? value) public bool TryGetValue(CliValueSymbol option, out object? value) => valueProvider.TryGetValue(option, out value); - - public CliValueResult? GetValueResult(CliValueSymbol valueSymbol) => ParseResult.GetValueResult(valueSymbol); - public void AddErrors(IEnumerable errors) { if (errors is not null) diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index 22e69dba3..142b165bc 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -72,8 +72,6 @@ private bool TryGetValueInternal(CliValueSymbol valueSymbol, out T? value) } } // !!! CRITICAL: All returns from this method should set the cache value to clear this pseudo-lock (use CacheAndReturn) - // TODO: Using the cache would only work if we gave up the cache returns being strongly typed. - // Consider instead a SetIsCalculating(valueSymbol); if (valueSymbol is CalculatedValue calculatedValueSymbol diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs index c9a0cf02e..4cf62796c 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs @@ -27,9 +27,25 @@ internal RelativeToSymbolsValueSource( Description = description; } + /// + /// The description that will be used in messages, such as value conditions + /// public override string? Description { get; } + + /// + /// The other symbols that this ValueSource depends on. + /// public IEnumerable OtherSymbols { get; } + + /// + /// If true, default values will not be used. + /// + // TODO: Find scenarios for good tests. This is based on intuition not a known scenario. Overall we are pretty aggressive about default values. public bool OnlyUserEnteredValues { get; } + + /// + /// The calculation that determines a single value, which might an instance of a complex type, based on the values provided. + /// public Func, (bool success, T? value)> Calculation { get; } /// From 81d2ca8503f15573c7656982ed344f565ee72f13 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Mon, 2 Sep 2024 08:42:37 -0400 Subject: [PATCH 7/9] Renaming: All value sources but constants include calculations --- .../System.CommandLine.Subsystems.Tests.csproj | 4 ---- .../ValueSourceTests.cs | 10 +++++----- .../Validation/ValidationContext.cs | 1 - ...oSymbolsValueSource.cs => CollectionValueSource.cs} | 4 ++-- ...alueSource.cs => EnvironmentVariableValueSource.cs} | 4 ++-- ...{AggregateValueSource.cs => FallbackValueSource.cs} | 8 ++++---- ...tiveToSymbolValueSource.cs => SymbolValueSource.cs} | 4 ++-- .../ValueSources/ValueSource.cs | 10 +++++----- 8 files changed, 20 insertions(+), 25 deletions(-) rename src/System.CommandLine.Subsystems/ValueSources/{RelativeToSymbolsValueSource.cs => CollectionValueSource.cs} (97%) rename src/System.CommandLine.Subsystems/ValueSources/{RelativeToEnvironmentVariableValueSource.cs => EnvironmentVariableValueSource.cs} (96%) rename src/System.CommandLine.Subsystems/ValueSources/{AggregateValueSource.cs => FallbackValueSource.cs} (86%) rename src/System.CommandLine.Subsystems/ValueSources/{RelativeToSymbolValueSource.cs => SymbolValueSource.cs} (96%) diff --git a/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj b/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj index e7f77c86c..c295972cc 100644 --- a/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj +++ b/src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj @@ -4,12 +4,8 @@ $(TargetFrameworkForNETSDK) false $(DefaultExcludesInProjectFolder);TestApps\** - Library enable annotations - False diff --git a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs index 87773b4b5..420530792 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs @@ -108,7 +108,7 @@ public void CalculatedValueSource_from_extension_produces_value() public void RelativeToSymbolValueSource_produces_value_that_was_set() { var option = new CliOption("-a"); - var valueSource = new RelativeToSymbolValueSource(option); + var valueSource = new SymbolValueSource(option); var found = valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value); @@ -137,7 +137,7 @@ public void RelativeToSymbolValueSource_implicitly_converted_produces_value_that public void RelativeToSymbolValueSource_from_extension_produces_value_that_was_set() { var option = new CliOption("-a"); - var valueSource = new RelativeToSymbolValueSource(option); + var valueSource = new SymbolValueSource(option); var found = valueSource.TryGetTypedValue(EmptyPipelineResult("-a 42", option), out var value); @@ -151,7 +151,7 @@ public void RelativeToSymbolValueSource_from_extension_produces_value_that_was_s public void RelativeToEnvironmentVariableValueSource_produces_value_that_was_set() { var envName = "SYSTEM_COMMANDLINE_TESTING"; - var valueSource = new RelativeToEnvironmentVariableValueSource(envName); + var valueSource = new EnvironmentVariableValueSource(envName); Environment.SetEnvironmentVariable(envName, "42"); var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); @@ -184,7 +184,7 @@ public void RelativeToEnvironmentVariableValueSource_from_extension_produces_val public void RelativeToSymbolValueSource_false_if_other_symbol_has_no_default_and_is_missing() { var option = new CliOption("-a"); - var valueSource = new RelativeToSymbolValueSource(option); + var valueSource = new SymbolValueSource(option); var found = valueSource.TryGetTypedValue(EmptyPipelineResult("", option), out var value); found.Should() @@ -197,7 +197,7 @@ public void RelativeToSymbolValueSource_false_if_other_symbol_has_no_default_and public void RelativeToEnvironmentVariable_false_if_environment_variable_missing() { var envName = "SYSTEM_COMMANDLINE_TESTING"; - var valueSource = new RelativeToEnvironmentVariableValueSource(envName); + var valueSource = new EnvironmentVariableValueSource(envName); var found = valueSource.TryGetTypedValue(EmptyPipelineResult(), out var value); diff --git a/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs b/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs index 3f22a1228..d75e64c74 100644 --- a/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs +++ b/src/System.CommandLine.Subsystems/Validation/ValidationContext.cs @@ -3,7 +3,6 @@ using System.CommandLine.Parsing; using System.CommandLine.ValueSources; -using static System.Runtime.InteropServices.JavaScript.JSType; namespace System.CommandLine.Validation; diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs similarity index 97% rename from src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs rename to src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs index 4cf62796c..875999796 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolsValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs @@ -12,10 +12,10 @@ namespace System.CommandLine.ValueSources; /// A delegate that returns the requested type. /// The description of this value, used to clarify the intent of the values that appear in error messages. // TODO: Do we want this to be an aggregate, such that you could build a type from other symbols, calcs and env variables. Ooo aahh -public sealed class RelativeToSymbolsValueSource +public sealed class CollectionValueSource : ValueSource { - internal RelativeToSymbolsValueSource( + internal CollectionValueSource( Func, (bool success, T? value)> calculation, bool onlyUserEnteredValues = false, string? description = null, diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/EnvironmentVariableValueSource.cs similarity index 96% rename from src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs rename to src/System.CommandLine.Subsystems/ValueSources/EnvironmentVariableValueSource.cs index 07faef3ff..d5f6aac67 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/EnvironmentVariableValueSource.cs @@ -11,10 +11,10 @@ namespace System.CommandLine.ValueSources; /// The name of then environment variable. Note that for some systems, this is case sensitive. /// A delegate that returns the requested type. If it is not specified, standard type conversions are used. /// The description of this value, used to clarify the intent of the values that appear in error messages. -public sealed class RelativeToEnvironmentVariableValueSource +public sealed class EnvironmentVariableValueSource : ValueSource { - internal RelativeToEnvironmentVariableValueSource( + internal EnvironmentVariableValueSource( string environmentVariableName, Func? calculation = null, string? description = null) diff --git a/src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/FallbackValueSource.cs similarity index 86% rename from src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs rename to src/System.CommandLine.Subsystems/ValueSources/FallbackValueSource.cs index 751b78979..f9c10a054 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/AggregateValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/FallbackValueSource.cs @@ -3,11 +3,11 @@ namespace System.CommandLine.ValueSources; -public sealed class AggregateValueSource : ValueSource +public sealed class FallbackValueSource : ValueSource { private List> valueSources = []; - internal AggregateValueSource(ValueSource firstSource, + internal FallbackValueSource(ValueSource firstSource, ValueSource secondSource, string? description = null, params ValueSource[] otherSources) @@ -45,10 +45,10 @@ internal static int GetPrecedence(ValueSource source) return source switch { SimpleValueSource => 0, - RelativeToSymbolValueSource => 1, + SymbolValueSource => 1, CalculatedValueSource => 2, //RelativeToConfigurationValueSource => 3, - RelativeToEnvironmentVariableValueSource => 4, + EnvironmentVariableValueSource => 4, _ => 5 }; } diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/SymbolValueSource.cs similarity index 96% rename from src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs rename to src/System.CommandLine.Subsystems/ValueSources/SymbolValueSource.cs index 68621ef6b..2beb6d5d5 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/SymbolValueSource.cs @@ -11,11 +11,11 @@ namespace System.CommandLine.ValueSources; /// The option or argument to return, with the calculation supplied if it is not null. /// A delegate that returns the requested type. /// The description of this value, used to clarify the intent of the values that appear in error messages. -public sealed class RelativeToSymbolValueSource +public sealed class SymbolValueSource : ValueSource { // TODO: API differences between this adn RelativeToSymbols are very annoying - internal RelativeToSymbolValueSource( + internal SymbolValueSource( CliValueSymbol otherSymbol, Func? calculation = null, bool onlyUserEnteredValues = false, diff --git a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs index ed2b81c3e..327b25057 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs @@ -34,25 +34,25 @@ public static ValueSource Create(CliValueSymbol otherSymbol, Func? calculation = null, bool userEnteredValueOnly = false, string? description = null) - => new RelativeToSymbolValueSource(otherSymbol, calculation, userEnteredValueOnly, description); + => new SymbolValueSource(otherSymbol, calculation, userEnteredValueOnly, description); public static ValueSource Create( Func, (bool success, T? value)> calculation, bool userEnteredValueOnly = false, string? description = null, params CliValueSymbol[] otherSymbols) - => new RelativeToSymbolsValueSource(calculation, userEnteredValueOnly, description, otherSymbols); + => new CollectionValueSource(calculation, userEnteredValueOnly, description, otherSymbols); public static ValueSource Create(ValueSource firstSource, ValueSource secondSource, string? description = null, params ValueSource[] otherSources) - => new AggregateValueSource(firstSource, secondSource, description, otherSources); + => new FallbackValueSource(firstSource, secondSource, description, otherSources); public static ValueSource CreateFromEnvironmentVariable(string environmentVariableName, Func? calculation = null, string? description = null) - => new RelativeToEnvironmentVariableValueSource(environmentVariableName, calculation, description); + => new EnvironmentVariableValueSource(environmentVariableName, calculation, description); } // TODO: Determine philosophy for custom value sources and whether they can build on existing sources. @@ -84,7 +84,7 @@ public override bool TryGetValue(PipelineResult pipelineResult, public static implicit operator ValueSource(T value) => new SimpleValueSource(value); public static implicit operator ValueSource(Func<(bool success, T? value)> calculated) => new CalculatedValueSource(calculated); - public static implicit operator ValueSource(CliValueSymbol symbol) => new RelativeToSymbolValueSource(symbol); + public static implicit operator ValueSource(CliValueSymbol symbol) => new SymbolValueSource(symbol); // Environment variable does not have an explicit operator, because converting to string was too broad } From 17be3a24b44a11f8be34ff34c308b03743f6f443 Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Mon, 2 Sep 2024 11:24:55 -0400 Subject: [PATCH 8/9] Better message for CollectionValueSource parameters --- .../ValueSources/CollectionValueSource.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs index 875999796..0b7262b60 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/CollectionValueSource.cs @@ -8,10 +8,9 @@ namespace System.CommandLine.ValueSources; /// If the calculation delegate is supplied, the returned value of the calculation is returned. /// /// The type to be returned, which is almost always the type of the symbol the ValueSource will be used for. -/// The option or argument to return, with the calculation supplied if it is not null. -/// A delegate that returns the requested type. +/// The , , or to include as sources. +/// A delegate that returns a value of the type of the collection source, which can be either a single value or a collection of values. /// The description of this value, used to clarify the intent of the values that appear in error messages. - // TODO: Do we want this to be an aggregate, such that you could build a type from other symbols, calcs and env variables. Ooo aahh public sealed class CollectionValueSource : ValueSource { From 9edf1cc9de59573d6303eda8bb6910e986c67aff Mon Sep 17 00:00:00 2001 From: Kathleen Dollard Date: Sat, 14 Sep 2024 05:50:06 -0700 Subject: [PATCH 9/9] Remove CalculateVaue and OtherValueSymbol support --- .../DirectiveSubsystemTests.cs | 1 - .../ErrorReportingFunctionalTests.cs | 7 -- .../ValueProviderTests.cs | 12 ++-- .../ValueSourceTests.cs | 2 - .../CalculatedValue.cs | 72 ------------------- .../Annotations/ValueAnnotations.cs | 5 -- .../ValueAnnotationExtensions.cs | 8 --- .../ValueProvider.cs | 8 +-- src/System.CommandLine/CliCommand.cs | 43 +++++------ 9 files changed, 26 insertions(+), 132 deletions(-) delete mode 100644 src/System.CommandLine.Subsystems/CalculatedValue.cs diff --git a/src/System.CommandLine.Subsystems.Tests/DirectiveSubsystemTests.cs b/src/System.CommandLine.Subsystems.Tests/DirectiveSubsystemTests.cs index 2dbdebb50..ce4df7976 100644 --- a/src/System.CommandLine.Subsystems.Tests/DirectiveSubsystemTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/DirectiveSubsystemTests.cs @@ -2,7 +2,6 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; -using System.CommandLine.Directives; using System.CommandLine.Parsing; using Xunit; diff --git a/src/System.CommandLine.Subsystems.Tests/ErrorReportingFunctionalTests.cs b/src/System.CommandLine.Subsystems.Tests/ErrorReportingFunctionalTests.cs index 47e4808c1..6e4637c3b 100644 --- a/src/System.CommandLine.Subsystems.Tests/ErrorReportingFunctionalTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ErrorReportingFunctionalTests.cs @@ -5,13 +5,6 @@ using System.CommandLine.Help; using System.CommandLine.Invocation; */ -using System.IO; -using FluentAssertions; -using Xunit; -using System.CommandLine.Tests.Utility; -using System.Linq; -using System.Threading.Tasks; - namespace System.CommandLine.Tests; public class ErrorReportingFunctionalTests diff --git a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs index 85c613f87..28d87a348 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueProviderTests.cs @@ -2,12 +2,9 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; -using System.CommandLine.Directives; using System.CommandLine.Parsing; using System.CommandLine.ValueSources; -using System.Runtime.Serialization; using Xunit; -using static System.CommandLine.Subsystems.Tests.TestData; namespace System.CommandLine.Subsystems.Tests; @@ -79,6 +76,7 @@ public void Default_value_is_used_when_user_did_not_enter_a_value() intOptionValue.Should().Be(42); } + /* Leaving calculated value tests in place, because they are examples of custom parser problems [Fact] public void Can_retrieve_calculated_value_() { @@ -99,6 +97,7 @@ public void Can_retrieve_calculated_value_() var intOptionValue = pipelineResult.GetValue(calcSymbol); intOptionValue.Should().Be(42); } + */ [Fact] public void Circular_default_value_dependency_throw() @@ -122,7 +121,7 @@ public void Circular_default_value_dependency_throw() } [Fact] - public void dotnet_nuget_why_can_retrieve_project_and_package() + public void Missing_value_throws_on_GetValue() { var opt1 = new CliOption("opt1"); var opt2 = new CliOption("opt2"); @@ -142,6 +141,9 @@ public void dotnet_nuget_why_can_retrieve_project_and_package() Assert.Throws(() => _ = pipelineResult.GetValue(opt1)); } + /* Leaving calculated value tests in place, because they are examples of custom parser problems + // dotnet nuget why has two parameters, where the first is optional. This is historic, and we must support it + // We also believe folks are using the old custom parser to create compound types - the Point tests here private (CliRootCommand root, CalculatedValue project, CalculatedValue package) CreateDotnetNugetWhyCli() { var whyArg = new CliArgument("whyArgs"); @@ -248,5 +250,5 @@ public void Can_make_point_from_two_options(string input, string expected) .Should() .Be(expected); } - + */ } diff --git a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs index 420530792..f12bdf170 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValueSourceTests.cs @@ -2,11 +2,9 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using FluentAssertions; -using Microsoft.VisualBasic.FileIO; using System.CommandLine.Parsing; using System.CommandLine.ValueSources; using Xunit; -using static System.Net.Mime.MediaTypeNames; namespace System.CommandLine.Subsystems.Tests; diff --git a/src/System.CommandLine.Subsystems/CalculatedValue.cs b/src/System.CommandLine.Subsystems/CalculatedValue.cs deleted file mode 100644 index cbaa61ca3..000000000 --- a/src/System.CommandLine.Subsystems/CalculatedValue.cs +++ /dev/null @@ -1,72 +0,0 @@ -using System.CommandLine.ValueSources; - -namespace System.CommandLine; - -/// -/// CalculatedValueSymbol lets ValueSource contribute to the data space of the application -/// with many standard features such as default values and the ability to participate in help. -/// -/// -/// Known scenarios: -/// -/// `dotnet nuget why`: where for historic reasons the first argument is optional and the second is required. -/// Completions: where the number of items to be displayed may come from an environment variable or a config file. -/// Compound types: where multiple value symbols contribute to the creation of a type, like -x and -y creating a point. -/// Reusing a value, for example to avoid multiple reads of a file -/// -/// CalculatedValueSymbol's should be available to subsystems, including invocation and conditions. -/// -public abstract class CalculatedValue : CliValueSymbol -{ - protected CalculatedValue(string name, ValueSource valueSource) - :base(name) - { - ValueSource = valueSource; - } - - /// - /// Create a CalculatedValue. - /// - /// The type of the value that can be retrieved via this calculated value. - // TODO: Provide name lookup of CalculatedValues - /// The name of the calculated value - /// The ValueSource used to retrieve the value. If there are defaults/fallbacks, this will be an aggregate value source. - /// - public static CalculatedValue CreateCalculatedValue(string name, ValueSource valueSource) - => new(name, valueSource); - - // TODO: This feels backwards. Should probably appear on the string array used as the data source. - public IEnumerable? AppearAs { get; set; } - - /// - /// The ValueSource used to retrieve the value. If there are defaults/fallbacks, this will be an - /// aggregate value source. - /// - public ValueSource ValueSource { get; } - - internal bool TryGetValue(PipelineResult pipelineResult, out T? calculatedValue) - { - if (ValueSource.TryGetValue(pipelineResult, out object? objectValue)) - { - calculatedValue = (T?)objectValue; - return true; - } - calculatedValue = default; - return false; - } -} - -/// -/// CalculatedValueSymbol lets ValueSource contribute to the data space of the application -/// with many standard features such as default values and the ability to participate in help. -/// -/// The value type of the symbol. -public class CalculatedValue : CalculatedValue -{ - internal CalculatedValue(string name, ValueSource valueSource) - : base(name, valueSource) - { } - - /// - public override Type ValueType { get; } = typeof(T); -} diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs index e610d9bf8..7695c363e 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/ValueAnnotations.cs @@ -36,9 +36,4 @@ public static class ValueAnnotations /// /// public static AnnotationId DefaultValueCalculation { get; } = new(Prefix, nameof(DefaultValueCalculation)); - - /// - /// - /// - public static AnnotationId CalculatedValues { get; } = new (Prefix, nameof(CalculatedValues)); } diff --git a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs index bb4d69269..c76e61c4e 100644 --- a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs @@ -33,12 +33,4 @@ public static bool TryGetDefault(this CliValueSymbol valueSymbol, out ValueSourc public static void SetDefault(this CliValueSymbol valueSymbol, ValueSource defaultValue) => valueSymbol.SetAnnotation(ValueAnnotations.DefaultValue, defaultValue); - public static void AddCalculatedValue(this CliCommand command, CalculatedValue calculatedValue) - { - if (!command.TryGetAnnotation>(ValueAnnotations.CalculatedValues, out var currentList)) - { - currentList = new List(); - } - currentList.Add(calculatedValue); - } } diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index 142b165bc..f6ec04af3 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -74,13 +74,7 @@ private bool TryGetValueInternal(CliValueSymbol valueSymbol, out T? value) // !!! CRITICAL: All returns from this method should set the cache value to clear this pseudo-lock (use CacheAndReturn) SetIsCalculating(valueSymbol); - if (valueSymbol is CalculatedValue calculatedValueSymbol - && calculatedValueSymbol.TryGetValue(pipelineResult, out value)) - { - return CacheAndReturn(valueSymbol, value, true); - } - if (valueSymbol is not CalculatedValue - && pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) + if (pipelineResult.ParseResult?.GetValueResult(valueSymbol) is { } valueResult) { value = valueResult.GetValue(); return CacheAndReturn(valueSymbol, value, true); diff --git a/src/System.CommandLine/CliCommand.cs b/src/System.CommandLine/CliCommand.cs index 27a28858d..18421e461 100644 --- a/src/System.CommandLine/CliCommand.cs +++ b/src/System.CommandLine/CliCommand.cs @@ -27,7 +27,6 @@ public class CliCommand : CliSymbol, IEnumerable internal AliasSet? _aliases; private ChildSymbolList? _arguments; private ChildSymbolList? _options; - private ChildSymbolList? _otherValueSymbols; private ChildSymbolList? _subcommands; // TODO: validators /* @@ -42,11 +41,11 @@ public class CliCommand : CliSymbol, IEnumerable /// The description of the command, shown in help. */ public CliCommand(string name)/*, string? description = null) */ - : base(name) + : base(name) { } -// TODO: help - //=> Description = description; + // TODO: help + //=> Description = description; /// /// Gets the child symbols. @@ -71,15 +70,13 @@ public IEnumerable Children /// public IList Arguments => _arguments ??= new(this); - internal bool HasArguments => _arguments?.Count > 0 ; + internal bool HasArguments => _arguments?.Count > 0; /// /// Represents all of the options for the command, inherited options that have been applied to any of the command's ancestors. /// // TODO: Consider value of lazy here. It sets up a desire to use awkward approach (HasOptions) for a perf win. Applies to Options and Subcommands also. - public IList Options => _options ??= new (this); - - public IList OtherSymbols => _otherValueSymbols ??= new(this); + public IList Options => _options ??= new(this); internal bool HasOptions => _options?.Count > 0; @@ -182,24 +179,24 @@ public void SetAction(Func> action) /// Adds a to the command. /// /// The option to add to the command. - public void Add(CliArgument argument) => Arguments.Add(argument); - + public void Add(CliArgument argument) => Arguments.Add(argument); + /// /// Adds a to the command. /// /// The option to add to the command. - public void Add(CliOption option) => Options.Add(option); + public void Add(CliOption option) => Options.Add(option); /// /// Adds a to the command. /// /// The Command to add to the command. - public void Add(CliCommand command) => Subcommands.Add(command); + public void Add(CliCommand command) => Subcommands.Add(command); // Hide from IntelliSense as it's only to support initializing via C# collection expression // More specific efficient overloads are available for all supported symbol types. //[DebuggerStepThrough] - [EditorBrowsable(EditorBrowsableState.Never)] + //[EditorBrowsable(EditorBrowsableState.Never)] public void Add(CliSymbol symbol) { if (symbol is CliCommand cmd) @@ -214,25 +211,21 @@ public void Add(CliSymbol symbol) { Add(command); } - else if (symbol is CliValueSymbol valueSymbol) - { - OtherSymbols.Add(valueSymbol); - } - else + else { // TODO: add a localized message here throw new ArgumentException(null, nameof(symbol)); } } -// TODO: umatched tokens - /* + // TODO: umatched tokens + /* /// /// Gets or sets a value that indicates whether unmatched tokens should be treated as errors. For example, /// if set to and an extra command or argument is provided, validation will fail. /// public bool TreatUnmatchedTokensAsErrors { get; set; } = true; -*/ + */ /// // Hide from IntelliSense as it's only to support C# collection initializer [DebuggerStepThrough] @@ -244,7 +237,7 @@ public void Add(CliSymbol symbol) [DebuggerStepThrough] [EditorBrowsable(EditorBrowsableState.Never)] IEnumerator IEnumerable.GetEnumerator() => Children.GetEnumerator(); -/* + /* /// /// Parses an array strings using the command. /// @@ -334,8 +327,8 @@ public override IEnumerable GetCompletions(CompletionContext con } return completions - .OrderBy(item => item.SortText.IndexOfCaseInsensitive(context.WordToComplete)) - .ThenBy(symbol => symbol.Label, StringComparer.OrdinalIgnoreCase); + .OrderBy(item => item.SortText.IndexOfCaseInsensitive(context.WordToComplete)) + .ThenBy(symbol => symbol.Label, StringComparer.OrdinalIgnoreCase); void AddCompletionsFor(CliSymbol identifier, AliasSet? aliases) { @@ -359,7 +352,7 @@ void AddCompletionsFor(CliSymbol identifier, AliasSet? aliases) } } } -*/ + */ internal bool EqualsNameOrAlias(string name) => Name.Equals(name, StringComparison.Ordinal) || (_aliases is not null && _aliases.Contains(name)); } }