Cleanup, notes, and a small fix

Author: KathleenDollard

OpenQuestions.md

@@ -0,0 +1,56 @@
+# 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
+

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

@@ -43,7 +43,7 @@ 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
@@ -56,7 +56,7 @@ public override void Execute(PipelineResult pipelineResult)
                 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/System.CommandLine.Subsystems.csproj

@@ -5,12 +5,18 @@
     <ImplicitUsings>enable</ImplicitUsings>
     <Nullable>enable</Nullable>
     <RootNamespace>System.CommandLine</RootNamespace>
+    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
   </PropertyGroup>
 
   <ItemGroup>
     <ProjectReference Include="..\System.CommandLine\System.CommandLine.csproj" />
   </ItemGroup>
 
+  <ItemGroup>
+    <!-- This was added to allow testing of ValueSource -->
+    <InternalsVisibleTo Include="System.CommandLine.Subsystems.Tests" />
+  </ItemGroup>
+
   <ItemGroup>
     <Compile Include="..\Common\Compat\**\*.cs" Link="Compat\%(RecursiveDir)%(Filename).%(Extension)" />
   </ItemGroup>

src/System.CommandLine.Subsystems/ValueSources/RelativeToEnvironmentVariableValueSource.cs

@@ -30,7 +30,7 @@ internal RelativeToEnvironmentVariableValueSource(
     /// <summary>
     /// The description of this value, used to clarify the intent of the values that appear in error messages.
     /// </summary>
-    public override string? Description { get; } 
+    public override string? Description { get; }
 
     public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value)
     {

src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs

@@ -22,6 +22,7 @@ internal ValueSource()
 
     // TODO: Should we use ToString() here?
     public abstract string? Description { get; }
+
     public static ValueSource<T> Create<T>(T value, string? description = null)
         => new SimpleValueSource<T>(value, description);
 
@@ -47,7 +48,7 @@ public static ValueSource<T> CreateFromEnvironmentVariable<T>(string environment
         => new RelativeToEnvironmentVariableValueSource<T>(environmentVariableName, calculation, description);
 }
 
-// TODO: Determine philosophy for custom value sources and whether tehy can buld on existing sources.
+// TODO: Determine philosophy for custom value sources and whether they can build on existing sources.
 public abstract class ValueSource<T> : ValueSource
 {
     /// <summary>
@@ -56,12 +57,13 @@ public abstract class ValueSource<T> : ValueSource
     /// <param name="pipelineResult">The current pipeline result.</param>
     /// <param name="value">An out parameter which contains the converted value, with the calculation applied, if it is found.</param>
     /// <returns>True if a value was found, otherwise false.</returns>
-    public abstract bool TryGetTypedValue(PipelineResult pipelineResult, 
-                                          [NotNullWhen(true)] out T? value);
+    // TODO: Determine whether this and `TryGetValue` should have NotNullWhen(true) attribute. Discussion in <reporoot>/OpenQuestions.md
+    public abstract bool TryGetTypedValue(PipelineResult pipelineResult,
+                                          out T? value);
 
     /// <inheritdoc/>
-    public override bool TryGetValue(PipelineResult pipelineResult, 
-                                     [NotNullWhen(true)] out object? value)
+    public override bool TryGetValue(PipelineResult pipelineResult,
+                                     out object? value)
     {
 
         if (TryGetTypedValue(pipelineResult, out T? newValue))

src/System.CommandLine/System.CommandLine.csproj

@@ -69,6 +69,7 @@
   </ItemGroup>
 
   <ItemGroup>
+    <!-- This was added to allow tests of tokenization -->
     <InternalsVisibleTo Include="System.CommandLine.Tests" />
   </ItemGroup>