Skip to content

Value handling cleanup #2486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main-powderhouse
Choose a base branch
from
5 changes: 2 additions & 3 deletions OpenQuestions.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,15 @@ Suggestion: Use internal constructors and leave conditions public

## Should `ValueCondition` be called `Condition`?

They may apply to commands.
They may apply to comm

Choose a reason for hiding this comment

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

That's just harder to understand.


## 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ 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 <see cref="IAnnotationProvider"/>.
/// </remarks>
public static bool TryGetDefault(this CliValueSymbol valueSymbol, out ValueSource? defaultValueSource)
=> valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out defaultValueSource);
public static ValueSource? GetDefault(this CliValueSymbol valueSymbol)
=> valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out var defaultValueSource)
? (ValueSource)defaultValueSource
: null;

/// <summary>
/// Sets the default value annotation on the <paramref name="option"/>
Expand Down
26 changes: 6 additions & 20 deletions src/System.CommandLine.Subsystems/ValueProvider.cs
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
// 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 System.CommandLine.Subsystems.Annotations;
using System.CommandLine.ValueSources;
using static System.Net.Mime.MediaTypeNames;

Choose a reason for hiding this comment

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

MediaTypeNames doesn't seem relevant here.


namespace System.CommandLine;

internal class ValueProvider
{
private struct CacheItem
{
internal object? Value;
internal bool WasFound;
internal bool IsCalculating;
}
private record struct CacheItem(object? Value, bool WasFound, bool IsCalculating)
{ }

private Dictionary<CliSymbol, CacheItem> cachedValues = [];
private PipelineResult pipelineResult;
Expand All @@ -22,17 +20,6 @@ public ValueProvider(PipelineResult pipelineResult)
this.pipelineResult = pipelineResult;
}

private void SetCachedValue(CliSymbol symbol, object? value, bool found)
{
// 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 void SetIsCalculating(CliSymbol symbol)
{
cachedValues[symbol] = new CacheItem()
Expand Down Expand Up @@ -79,7 +66,7 @@ private bool TryGetValueInternal<T>(CliValueSymbol valueSymbol, out T? value)
value = valueResult.GetValue<T>();
return CacheAndReturn(valueSymbol, value, true);
}
if (valueSymbol.TryGetDefault(out ValueSource? defaultValueSource))
if (valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValue, out var defaultValueSource))
{
if (defaultValueSource is not ValueSource<T> typedDefaultValueSource)
{
Expand All @@ -90,13 +77,12 @@ private bool TryGetValueInternal<T>(CliValueSymbol valueSymbol, out T? value)
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
value = default;
return CacheAndReturn(valueSymbol, value, false);

bool CacheAndReturn(CliValueSymbol valueSymbol, T? valueToCache, bool valueFound)
{
SetCachedValue(valueSymbol, valueToCache, valueFound);
cachedValues[valueSymbol] = new CacheItem(valueToCache, valueFound, false);
return valueFound;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@

namespace System.CommandLine.ValueSources;

// TODO: Consider creating a set of classes with different arity: T1 and T1, T2 and T1, T2, T3, etc.

Choose a reason for hiding this comment

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

This would be easier to parse if written with angle brackets

// TODO: Consider creating a set of classes with different arity: <T1> and <T1, T2> and <T1, T2, T3>, etc.


/// <summary>
/// <see cref="ValueSource"/> that returns the value of the specified other symbol.
/// If the calculation delegate is supplied, the returned value of the calculation is returned.
/// <see cref="ValueSource"/> that returns the value value calculated from a set of other symbols.
/// The calculation must be supplied. the returned value of the calculation is returned.
/// </summary>
/// <typeparam name="T">The type to be returned, which is almost always the type of the symbol the ValueSource will be used for.</typeparam>
/// <param name="otherSymbols">The <see cref="CliOption">, <see cref="CliArgument"/>, or <see cref="CalculatedValue"/> to include as sources.</param>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace System.CommandLine.ValueSources;
public sealed class SymbolValueSource<T>
: ValueSource<T>
{
// TODO: API differences between this adn RelativeToSymbols are very annoying
// TODO: API differences between this and RelativeToSymbols are very annoying
internal SymbolValueSource(
CliValueSymbol otherSymbol,
Func<object?, (bool success, T? value)>? calculation = null,
Expand Down
5 changes: 1 addition & 4 deletions src/System.CommandLine/CliCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,7 @@ public void SetAction(Func<ParseResult, CancellationToken, Task<int>> 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]
//[EditorBrowsable(EditorBrowsableState.Never)]
[EditorBrowsable(EditorBrowsableState.Never)]
public void Add(CliSymbol symbol)
{
if (symbol is CliCommand cmd)
Expand Down Expand Up @@ -228,13 +227,11 @@ public void Add(CliSymbol symbol)
*/
/// <inheritdoc />
// Hide from IntelliSense as it's only to support C# collection initializer
[DebuggerStepThrough]
[EditorBrowsable(EditorBrowsableState.Never)]
IEnumerator IEnumerable.GetEnumerator() => Children.GetEnumerator();

/// <inheritdoc />
// Hide from IntelliSense as it's only to support initializing via C# collection expression
[DebuggerStepThrough]
[EditorBrowsable(EditorBrowsableState.Never)]
IEnumerator<CliSymbol> IEnumerable<CliSymbol>.GetEnumerator() => Children.GetEnumerator();
/*
Expand Down