Merge pull request #2477 from KathleenDollard/symbolvalue-usage-validation-valueprovider

ValidationSubsystem

Author: KathleenDollard

OpenQuestions.md

@@ -0,0 +1,59 @@
+# Open questions
+
+Please also include TODO in appropriate locations. This is intended as a wrap up.
+
+Also, include whether the question is to add or remove something, and add date/initials
+
+## NotNulWhen on TryGetValue in ValueSource and ValueProvider
+
+Things to consider:
+
+* Within System.CommandLine all TryGetValue should probably be the same
+* TryGetValue on dictionary can return null
+* For nullable values, the actual value can be null
+* For nullable or non-nullable ref types, the default for the type is null
+* Allowing null out values keeps a single meaning to "not found" and allows "found but null". Conflating these blocks expressing which happened 
+
+The recovery is the same as with Dictionary.TryGetValue. The first line of the block that handles the return Boolean is a guard.
+
+## The extensibility story for ValueSource
+
+The proposal and current code seal our value sources and expect people to make additional ones based on ValueSource. The classes are public and sealed, the constructors are internal.
+
+Reasons to reconsider: Aggregate value source has a logical precedence or an as entered one. If someone adds a new value source, it is always last in the logic precedence.There are likely to be other similar cases.
+
+Possible resolution: Have this be case by case and allow aggregate values to be unsealed and have a mechanism for overriding. Providing a non-inheritance based solution could make this look like a normal operation when it is a rare one.
+
+## Contexts [RESOLVED]
+
+We had two different philosophies at different spots in subsystems. "Give folks everything they might need" and "Give folks only what we know they need".
+
+The first probably means we pass around `PipelineResult`. The second means that each purpose needs a special context. Sharing contexts is likely to mean that something will be added to one context that is unneeded by the other. Known expected contexts are:
+
+- `AnnotationProviderContext` 
+- `ValueSourceContext` 
+- `ValidationContext` (includes ability to report diagnostics)
+- `CompletionContext` 
+- `HelpContext` 
+
+## Which contexts should allow diagnostic reporting?
+
+## Should we have both Validators and IValidator on Conditions? [RESOLVED]
+
+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.)
+
+When present, custom validators have precedence. There is no cost when they are not present.
+
+## Should conditions be public
+
+Since there are factory methods and validators could still access them, current behavior could be supported with internal conditions.
+
+However, the point of conditions is that they are a statement about the symbol and not an implementation. They are known to be used by completions and completions are expected to be extended. Thus, to get the values held in the condition (such as environment variable name) need to be available outside the external scope.
+
+Suggestion: Use internal constructors and leave conditions public
+
+## Should `ValueCondition` be called `Condition`?
+
+They may apply to commands.

System.CommandLine.sln

@@ -16,6 +16,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
 		Directory.Packages.props = Directory.Packages.props
 		global.json = global.json
 		LICENSE.md = LICENSE.md
+		OpenQuestions.md = OpenQuestions.md
 		README.md = README.md
 		restore.cmd = restore.cmd
 		restore.sh = restore.sh

src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs

@@ -10,7 +10,7 @@ internal class AlternateSubsystems
     {
         internal class AlternateVersion : VersionSubsystem
         {
-            protected override void Execute(PipelineResult pipelineResult)
+            public override void Execute(PipelineResult pipelineResult)
             {
                 pipelineResult.ConsoleHack.WriteLine($"***{CliExecutable.ExecutableVersion}***");
                 pipelineResult.SetSuccess();
@@ -28,7 +28,7 @@ public VersionThatUsesHelpData(CliSymbol symbol)
 
             private CliSymbol Symbol { get; }
 
-            protected override void Execute(PipelineResult pipelineResult)
+            public override void Execute(PipelineResult pipelineResult)
             {
                 TryGetAnnotation(Symbol, HelpAnnotations.Description, out string? description);
                 pipelineResult.ConsoleHack.WriteLine(description);
@@ -43,20 +43,20 @@ internal class VersionWithInitializeAndTeardown : VersionSubsystem
             internal bool ExecutionWasRun;
             internal bool TeardownWasRun;
 
-            protected override void Initialize(InitializationContext context)
+            protected internal override void Initialize(InitializationContext context)
             {
                 base.Initialize(context);
                 // marker hack needed because ConsoleHack not available in initialization
                 InitializationWasRun = true;
             }
 
-            protected override void Execute(PipelineResult pipelineResult)
+            public override void Execute(PipelineResult pipelineResult)
             {
                 ExecutionWasRun = true;
                 base.Execute(pipelineResult);
             }
 
-            protected override void TearDown(PipelineResult pipelineResult)
+            protected internal override void TearDown(PipelineResult pipelineResult)
             {
                 TeardownWasRun = true;
                 base.TearDown(pipelineResult);

src/System.CommandLine.Subsystems.Tests/System.CommandLine.Subsystems.Tests.csproj

@@ -32,6 +32,8 @@
     -->
     <Compile Include="AlternateSubsystems.cs" />
     <Compile Include="Constants.cs" />
+    <Compile Include="ValueSourceTests.cs" />
+    <Compile Include="ValidationSubsystemTests.cs" />
     <Compile Include="ValueSubsystemTests.cs" />
     <Compile Include="ResponseSubsystemTests.cs" />
     <Compile Include="DirectiveSubsystemTests.cs" />

src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs

@@ -0,0 +1,187 @@
+// 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.
+
+using FluentAssertions;
+using System.CommandLine.Parsing;
+using System.CommandLine.ValueSources;
+using Xunit;
+
+namespace System.CommandLine.Subsystems.Tests;
+
+public class ValidationSubsystemTests
+{
+    // Running exactly the same code is important here because missing a step will result in a false positive. Ask me how I know
+    private CliOption GetOptionWithSimpleRange<T>(T lowerBound, T upperBound)
+        where T : IComparable<T>
+    {
+        var option = new CliOption<int>("--intOpt");
+        option.SetRange(lowerBound, upperBound);
+        return option;
+    }
+
+    private CliOption GetOptionWithRangeBounds<T>(ValueSource<T> lowerBound, ValueSource<T> upperBound)
+        where T : IComparable<T>
+    {
+        var option = new CliOption<int>("--intOpt");
+        option.SetRange(lowerBound, upperBound);
+        return option;
+    }
+
+    private PipelineResult ExecutedPipelineResultForRangeOption(CliOption option, string input)
+    {
+        var command = new CliRootCommand { option };
+        return ExecutedPipelineResultForCommand(command, input);
+    }
+
+    private PipelineResult ExecutedPipelineResultForCommand(CliCommand command, string input)
+    {
+        var validationSubsystem = ValidationSubsystem.Create();
+        var parseResult = CliParser.Parse(command, input, new CliConfiguration(command));
+        var pipelineResult = new PipelineResult(parseResult, input, Pipeline.CreateEmpty());
+        validationSubsystem.Execute(pipelineResult);
+        return pipelineResult;
+    }
+
+    [Fact]
+    public void Int_values_in_specified_range_do_not_have_errors()
+    {
+        var option = GetOptionWithSimpleRange(0, 50);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Int_values_above_upper_bound_report_error()
+    {
+        var option = GetOptionWithSimpleRange(0, 5);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+    [Fact]
+    public void Int_below_lower_bound_report_error()
+    {
+        var option = GetOptionWithSimpleRange(0, 5);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt -42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+    [Fact]
+    public void Int_values_on_lower_range_bound_do_not_report_error()
+    {
+        var option = GetOptionWithSimpleRange(42, 50);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Int_values_on_upper_range_bound_do_not_report_error()
+    {
+        var option = GetOptionWithSimpleRange(0, 42);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Values_below_calculated_lower_bound_report_error()
+    {
+        var option = GetOptionWithRangeBounds<int>(ValueSource.Create(() => (true, 1)), 50);
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 0");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+
+    [Fact]
+    public void Values_within_calculated_range_do_not_report_error()
+    {
+        var option = GetOptionWithRangeBounds(ValueSource<int>.Create(() => (true, 1)), ValueSource<int>.Create(() => (true, 50)));
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Values_above_calculated_upper_bound_report_error()
+    {
+        var option = GetOptionWithRangeBounds(0, ValueSource<int>.Create(() => (true, 40)));
+
+        var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+    [Fact]
+    public void Values_below_relative_lower_bound_report_error()
+    {
+        var otherOption = new CliOption<int>("-a");
+        var option = GetOptionWithRangeBounds(ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50);
+        var command = new CliCommand("cmd") { option, otherOption };
+
+        var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 0 -a 0");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+
+    [Fact]
+    public void Values_within_relative_range_do_not_report_error()
+    {
+        var otherOption = new CliOption<int>("-a");
+        var option = GetOptionWithRangeBounds(ValueSource<int>.Create(otherOption, o => (true, (int)o + 1)), ValueSource<int>.Create(otherOption, o => (true, (int)o + 10)));
+        var command = new CliCommand("cmd") { option, otherOption };
+
+        var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 11 -a 3");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Values_above_relative_upper_bound_report_error()
+    {
+        var otherOption = new CliOption<int>("-a");
+        var option = GetOptionWithRangeBounds(0, ValueSource<int>.Create(otherOption, o => (true, (int)o + 10)));
+        var command = new CliCommand("cmd") { option, otherOption };
+
+        var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 9 -a -2");
+
+        pipelineResult.Should().NotBeNull();
+        pipelineResult.GetErrors().Should().HaveCount(1);
+        var error = pipelineResult.GetErrors().First();
+        // TODO: Create test mechanism for CliDiagnostics
+    }
+
+
+}