From bc885e722b42cae138b1ea5afd1ecf30755840bb Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Tue, 17 Sep 2024 23:11:07 -0400 Subject: [PATCH 1/6] Replace per-subsystem annotation providers with pipeline annotation resolver This simplifies things and means that subsystem authors have one centralized place to get annotation values, instead of having to know which subsytem to use. --- .../AlternateSubsystems.cs | 10 +- .../CompletionSubsystem.cs | 4 +- .../Directives/DiagramSubsystem.cs | 4 +- .../Directives/DirectiveSubsystem.cs | 4 +- .../Directives/ResponseSubsystem.cs | 2 +- .../ErrorReportingSubsystem.cs | 4 +- .../HelpAnnotationExtensions.cs | 21 ++- .../HelpSubsystem.cs | 7 +- .../InvocationSubsystem.cs | 4 +- src/System.CommandLine.Subsystems/Pipeline.cs | 31 ++++- .../Annotations/AnnotationResolver.cs | 126 ++++++++++++++++++ ...tionStorageExtensions.AnnotationStorage.cs | 2 +- .../AnnotationStorageExtensions.cs | 44 +++--- .../Annotations/AnnotationTypeException.cs | 2 +- .../Subsystems/CliSubsystem.cs | 71 +--------- .../Subsystems/IAnnotationProvider.cs | 1 + .../ValidationSubsystem.cs | 4 +- .../ValueConditionAnnotationExtensions.cs | 28 +++- .../VersionSubsystem.cs | 4 +- 19 files changed, 250 insertions(+), 123 deletions(-) create mode 100644 src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs diff --git a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs index 5677470b03..864ec95a18 100644 --- a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs +++ b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs @@ -30,7 +30,7 @@ public VersionThatUsesHelpData(CliSymbol symbol) public override void Execute(PipelineResult pipelineResult) { - TryGetAnnotation(Symbol, HelpAnnotations.Description, out string? description); + pipelineResult.Pipeline.Annotations.TryGet(Symbol, HelpAnnotations.Description, out string? description); pipelineResult.ConsoleHack.WriteLine(description); pipelineResult.AlreadyHandled = true; pipelineResult.SetSuccess(); @@ -63,12 +63,12 @@ protected internal override void TearDown(PipelineResult pipelineResult) } } - internal class StringDirectiveSubsystem(IAnnotationProvider? annotationProvider = null) - : DirectiveSubsystem("other", SubsystemKind.Diagram, annotationProvider) + internal class StringDirectiveSubsystem() + : DirectiveSubsystem("other", SubsystemKind.Diagram) { } - internal class BooleanDirectiveSubsystem(IAnnotationProvider? annotationProvider = null) - : DirectiveSubsystem("diagram", SubsystemKind.Diagram, annotationProvider) + internal class BooleanDirectiveSubsystem() + : DirectiveSubsystem("diagram", SubsystemKind.Diagram) { } } diff --git a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs index 8fd0156ba0..67039d2f08 100644 --- a/src/System.CommandLine.Subsystems/CompletionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/CompletionSubsystem.cs @@ -17,8 +17,8 @@ namespace System.CommandLine; public class CompletionSubsystem : CliSubsystem { - public CompletionSubsystem(IAnnotationProvider? annotationProvider = null) - : base(CompletionAnnotations.Prefix, SubsystemKind.Completion, annotationProvider) + public CompletionSubsystem() + : base(CompletionAnnotations.Prefix, SubsystemKind.Completion) { } // TODO: Figure out trigger for completions diff --git a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs index 791290a097..421b10851f 100644 --- a/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/DiagramSubsystem.cs @@ -6,8 +6,8 @@ namespace System.CommandLine.Directives; -public class DiagramSubsystem(IAnnotationProvider? annotationProvider = null) - : DirectiveSubsystem("diagram", SubsystemKind.Diagram, annotationProvider) +public class DiagramSubsystem() + : DirectiveSubsystem("diagram", SubsystemKind.Diagram) { //protected internal override bool GetIsActivated(ParseResult? parseResult) // => parseResult is not null && option is not null && parseResult.GetValue(option); diff --git a/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs index efb51bceee..3fb43e18ea 100644 --- a/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/DirectiveSubsystem.cs @@ -13,8 +13,8 @@ public abstract class DirectiveSubsystem : CliSubsystem public string Id { get; } public Location? Location { get; private set; } - public DirectiveSubsystem(string name, SubsystemKind kind, IAnnotationProvider? annotationProvider = null, string? id = null) - : base(name, kind, annotationProvider: annotationProvider) + public DirectiveSubsystem(string name, SubsystemKind kind, string? id = null) + : base(name, kind) { Id = id ?? name; } diff --git a/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs b/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs index c626348e28..421c89256a 100644 --- a/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Directives/ResponseSubsystem.cs @@ -7,7 +7,7 @@ namespace System.CommandLine.Directives; public class ResponseSubsystem() - : CliSubsystem("Response", SubsystemKind.Response, null) + : CliSubsystem("Response", SubsystemKind.Response) { public bool Enabled { get; set; } diff --git a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs index 7c7432616b..27f43ab9f2 100644 --- a/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ErrorReportingSubsystem.cs @@ -15,8 +15,8 @@ namespace System.CommandLine; /// public class ErrorReportingSubsystem : CliSubsystem { - public ErrorReportingSubsystem(IAnnotationProvider? annotationProvider = null) - : base(ErrorReportingAnnotations.Prefix, SubsystemKind.ErrorReporting, annotationProvider) + public ErrorReportingSubsystem() + : base(ErrorReportingAnnotations.Prefix, SubsystemKind.ErrorReporting) { } protected internal override bool GetIsActivated(ParseResult? parseResult) diff --git a/src/System.CommandLine.Subsystems/HelpAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/HelpAnnotationExtensions.cs index 8cb06c594d..82b37c3dee 100644 --- a/src/System.CommandLine.Subsystems/HelpAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/HelpAnnotationExtensions.cs @@ -40,11 +40,28 @@ public static void SetDescription(this TSymbol symbol, string descripti /// The symbol /// The symbol description if any, otherwise /// - /// This is intended to be called by CLI authors. Subsystems should instead call , - /// values from the subsystem's . + /// This is intended to be called by CLI authors. Subsystem authors should instead call + /// to get values from + /// the pipeline's , which takes dynamic providers into account. /// public static string? GetDescription(this TSymbol symbol) where TSymbol : CliSymbol { return symbol.GetAnnotationOrDefault(HelpAnnotations.Description); } + + /// + /// Get the help description for the from the , + /// which takes dynamic providers into account. + /// + /// The type of the symbol + /// The symbol + /// The symbol description if any, otherwise + /// + /// This is intended to be called by subsystem authors. CLI authors should instead call + /// to get the value associated directly with the symbol. + /// + public static string? GetDescription(this AnnotationResolver resolver, TSymbol symbol) where TSymbol : CliSymbol + { + return resolver.GetAnnotationOrDefault(symbol, HelpAnnotations.Description); + } } diff --git a/src/System.CommandLine.Subsystems/HelpSubsystem.cs b/src/System.CommandLine.Subsystems/HelpSubsystem.cs index ca4d99c2ae..29dd06aff4 100644 --- a/src/System.CommandLine.Subsystems/HelpSubsystem.cs +++ b/src/System.CommandLine.Subsystems/HelpSubsystem.cs @@ -15,8 +15,8 @@ namespace System.CommandLine; // var command = new CliCommand("greet") // .With(help.Description, "Greet the user"); // -public class HelpSubsystem(IAnnotationProvider? annotationProvider = null) - : CliSubsystem(HelpAnnotations.Prefix, SubsystemKind.Help, annotationProvider) +public class HelpSubsystem() + : CliSubsystem(HelpAnnotations.Prefix, SubsystemKind.Help) { /// /// Gets the help option, which allows the user to customize @@ -44,7 +44,4 @@ public override void Execute(PipelineResult pipelineResult) pipelineResult.ConsoleHack.WriteLine("Help me!"); pipelineResult.SetSuccess(); } - - public bool TryGetDescription(CliSymbol symbol, out string? description) - => TryGetAnnotation(symbol, HelpAnnotations.Description, out description); } diff --git a/src/System.CommandLine.Subsystems/InvocationSubsystem.cs b/src/System.CommandLine.Subsystems/InvocationSubsystem.cs index a451065831..4dec80e8b1 100644 --- a/src/System.CommandLine.Subsystems/InvocationSubsystem.cs +++ b/src/System.CommandLine.Subsystems/InvocationSubsystem.cs @@ -6,6 +6,6 @@ namespace System.CommandLine; -public class InvocationSubsystem(IAnnotationProvider? annotationProvider = null) - : CliSubsystem(InvocationAnnotations.Prefix, SubsystemKind.Invocation, annotationProvider) +public class InvocationSubsystem() + : CliSubsystem(InvocationAnnotations.Prefix, SubsystemKind.Invocation) {} diff --git a/src/System.CommandLine.Subsystems/Pipeline.cs b/src/System.CommandLine.Subsystems/Pipeline.cs index 837fdea398..5704bc45bf 100644 --- a/src/System.CommandLine.Subsystems/Pipeline.cs +++ b/src/System.CommandLine.Subsystems/Pipeline.cs @@ -4,6 +4,7 @@ using System.CommandLine.Directives; using System.CommandLine.Parsing; using System.CommandLine.Subsystems; +using System.CommandLine.Subsystems.Annotations; namespace System.CommandLine; @@ -17,6 +18,7 @@ public partial class Pipeline private readonly PipelinePhase invocationPhase = new(SubsystemKind.Invocation); private readonly PipelinePhase errorReportingPhase = new(SubsystemKind.ErrorReporting); private readonly IEnumerable phases; + private readonly List annotationProviders; /// /// Creates an instance of the pipeline using standard features. @@ -34,14 +36,15 @@ public static Pipeline Create(HelpSubsystem? help = null, VersionSubsystem? version = null, CompletionSubsystem? completion = null, DiagramSubsystem? diagram = null, - ErrorReportingSubsystem? errorReporting = null) - => new() + ErrorReportingSubsystem? errorReporting = null, + IEnumerable? annotationProviders = null) + => new(annotationProviders) { Help = help ?? new HelpSubsystem(), Version = version ?? new VersionSubsystem(), Completion = completion ?? new CompletionSubsystem(), Diagram = diagram ?? new DiagramSubsystem(), - ErrorReporting = errorReporting ?? new ErrorReportingSubsystem(), + ErrorReporting = errorReporting ?? new ErrorReportingSubsystem() }; /// @@ -54,7 +57,7 @@ public static Pipeline Create(HelpSubsystem? help = null, public static Pipeline CreateEmpty() => new(); - private Pipeline() + private Pipeline(IEnumerable? annotationProviders = null) { Response = new ResponseSubsystem(); Invocation = new InvocationSubsystem(); @@ -68,6 +71,11 @@ private Pipeline() diagramPhase, completionPhase, helpPhase, versionPhase, validationPhase, invocationPhase, errorReportingPhase ]; + + this.annotationProviders = annotationProviders is not null + ? [..annotationProviders] + : []; + Annotations = new(this.annotationProviders); } /// @@ -186,6 +194,21 @@ public ErrorReportingSubsystem? ErrorReporting /// public ResponseSubsystem Response { get; } + /// + /// Gets the annotation resolver + /// + public AnnotationResolver Annotations { get; } + + /// + /// Gets the list of annotation providers registered with the pipeline + /// + public IReadOnlyList AnnotationProviders => annotationProviders; + + /// + /// Adds an annotation provider to the pipeline + /// + public void AddAnnotationProvider(IAnnotationProvider annotationProvider) => annotationProviders.Add(annotationProvider); + public ParseResult Parse(CliConfiguration configuration, string rawInput) => Parse(configuration, CliParser.SplitCommandLine(rawInput).ToArray()); diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs new file mode 100644 index 0000000000..6831a75876 --- /dev/null +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs @@ -0,0 +1,126 @@ +// 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.Diagnostics.CodeAnalysis; + +namespace System.CommandLine.Subsystems.Annotations; + +/// +/// Provides a resolved annotation value for a given , +/// taking into account both the values stored directly on the symbol via extension methods +/// and any values from providers. +/// +/// +/// The will be enumerated each time an annotation value is requested. +/// It may be modified after the resolver is created. +/// +public class AnnotationResolver(ICollection providers) +{ + private readonly IEnumerable providers = providers ?? throw new ArgumentNullException(nameof(providers)); + + /// + /// Attempt to retrieve the 's value for the annotation . This will check any + /// annotation providers that were passed to the constructor, and the internal per-symbol annotation storage. + /// + /// + /// The expected type of the annotation value. If the type does not match, a will be thrown. + /// If the annotation allows multiple types for its values, and a type parameter cannot be determined statically, + /// use to access the annotation value without checking its type. + /// + /// The symbol the value is attached to + /// + /// The identifier for the annotation value to be retrieved. + /// For example, the annotation identifier for the help description is . + /// + /// An out parameter to contain the result + /// True if successful + /// + /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation + /// ID should also define an accessor extension method on extension method + /// on that subsystem authors can use to access the annotation value, such as + /// . + /// + /// If the annotation value does not have a single expected type for this symbol, use the + /// overload instead. + /// + /// + public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out TValue? value) + { + foreach (var provider in providers) + { + if (provider.TryGet(symbol, annotationId, out object? rawValue)) + { + if (rawValue is TValue expectedTypeValue) + { + value = expectedTypeValue; + return true; + } + throw new AnnotationTypeException(annotationId, typeof(TValue), rawValue?.GetType(), provider); + } + } + + return symbol.TryGetAnnotation(annotationId, out value); + } + + /// + /// Attempt to retrieve the 's value for the annotation . This will check any + /// annotation providers that were passed to the constructor, and the internal per-symbol annotation storage. + /// + /// The symbol the value is attached to + /// + /// The identifier for the annotation value to be retrieved. + /// For example, the annotation identifier for the help description is . + /// + /// An out parameter to contain the result + /// True if successful + /// + /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation + /// ID should also define an accessor extension method on extension method + /// on that subsystem authors can use to access the annotation value, such as + /// . + /// + /// If the expected type of the annotation value is known, use the + /// overload instead. + /// + /// + public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out object? value) + { + foreach (var provider in providers) + { + if (provider.TryGet(symbol, annotationId, out value)) + { + return true; + } + } + + return symbol.TryGetAnnotation(annotationId, out value); + } + + /// + /// Attempt to retrieve the 's value for the annotation . This will check any + /// annotation providers that were passed to the constructor, and the internal per-symbol annotation storage. If the + /// annotation value is not found, the default value for will be returned. + /// + /// The type of the annotation value + /// The symbol that is annotated + /// + /// The identifier for the annotation. For example, the annotation identifier for the help description + /// is . + /// + /// The annotation value, if successful, otherwise default + /// + /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation + /// ID should also define an accessor extension method on extension method + /// on that subsystem authors can use to access the annotation value, such as + /// . + /// + public TValue? GetAnnotationOrDefault(CliSymbol symbol, AnnotationId annotationId) + { + if (TryGet(symbol, annotationId, out TValue? value)) + { + return value; + } + + return default; + } +} \ No newline at end of file diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.AnnotationStorage.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.AnnotationStorage.cs index 58341d3bf1..c9cc4a7031 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.AnnotationStorage.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.AnnotationStorage.cs @@ -7,7 +7,7 @@ namespace System.CommandLine.Subsystems.Annotations; partial class AnnotationStorageExtensions { - class AnnotationStorage : IAnnotationProvider + class AnnotationStorage { record struct AnnotationKey(CliSymbol symbol, string prefix, string id) { diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs index 5a8a4be569..6e2a65cf56 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs @@ -107,9 +107,10 @@ public static TSymbol WithAnnotation(this TSymbol symbol, AnnotationId /// This is intended to be called by the implementation of specialized ID-specific accessors for CLI authors such as . /// /// - /// Subsystems should not call it directly, as it does not account for values from the subsystem's . They should instead call - /// or an ID-specific accessor on the subsystem such - /// . + /// Subsystems should not call this directly, as it does not account for values from the pipeline's . + /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// or an ID-specific + /// extension method such as . /// /// public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out TValue? value) @@ -129,24 +130,29 @@ public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId } /// - /// Attempts to get the value for the annotation associated with the in the internal annotation - /// storage used to store values set via . + /// Attempts to get the value for the annotation associated with the + /// in the internal annotation storage used to store values set via + /// . /// /// The symbol that is annotated /// - /// The identifier for the annotation. For example, the annotation identifier for the help description is . + /// The identifier for the annotation. For example, the annotation identifier for the help + /// description is . /// /// The annotation value, if successful, otherwise default /// True if successful /// - /// If the expected type of the annotation value is known, use the overload instead. + /// If the expected type of the annotation value is known, use the + /// overload instead. /// - /// This is intended to be called by the implementation of specialized ID-specific accessors for CLI authors such as . + /// This is intended to be called by the implementation of specialized ID-specific accessors for + /// CLI authors such as . /// /// - /// Subsystems should not call it directly, as it does not account for values from the subsystem's . They should instead call - /// or an ID-specific accessor on the subsystem such - /// . + /// Subsystems should not call this directly, as it does not account for values from the pipeline's . + /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// or an ID-specific + /// extension method such as . /// /// public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out object? value) @@ -161,20 +167,22 @@ public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId annotati } /// - /// Attempts to get the value for the annotation associated with the in the internal annotation - /// storage used to store values set via . + /// Attempts to get the value for the annotation associated with the + /// in the internal annotation storage used to store values set via + /// . /// /// The type of the annotation value /// The symbol that is annotated /// - /// The identifier for the annotation. For example, the annotation identifier for the help description is . + /// The identifier for the annotation. For example, the annotation identifier for the help description + /// is . /// /// The annotation value, if successful, otherwise default /// - /// This is intended to be called by specialized ID-specific accessors for CLI authors such as . - /// Subsystems should not call it, as it does not account for values from the subsystem's . They should instead call - /// or an ID-specific accessor on the subsystem such - /// . + /// Subsystems should not call this directly, as it does not account for values from the pipeline's . + /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// or an ID-specific + /// extension method such as . /// public static TValue? GetAnnotationOrDefault(this CliSymbol symbol, AnnotationId annotationId) { diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs index 4d1a19aded..313a274ad1 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs @@ -30,4 +30,4 @@ public override string Message $"This may be an authoring error in a typed annotation accessor, or the annotation may have be stored directly with the incorrect type, bypassing the typed accessors."; } } -} \ No newline at end of file +} diff --git a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs index 49e6f98af7..640a0cebf2 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/CliSubsystem.cs @@ -12,10 +12,9 @@ namespace System.CommandLine.Subsystems; /// public abstract class CliSubsystem { - protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProvider? annotationProvider) + protected CliSubsystem(string name, SubsystemKind subsystemKind) { Name = name; - _annotationProvider = annotationProvider; Kind = subsystemKind; } @@ -30,74 +29,6 @@ protected CliSubsystem(string name, SubsystemKind subsystemKind, IAnnotationProv public SubsystemKind Kind { get; } public AddToPhaseBehavior RecommendedAddToPhaseBehavior { get; } - private readonly IAnnotationProvider? _annotationProvider; - - /// - /// Attempt to retrieve the 's value for the annotation . This will check the - /// annotation provider that was passed to the subsystem constructor, and the internal annotation storage. - /// - /// - /// The expected type of the annotation value. If the type does not match, a will be thrown. - /// If the annotation allows multiple types for its values, and a type parameter cannot be determined statically, - /// use to access the annotation value without checking its type. - /// - /// The symbol the value is attached to - /// - /// The identifier for the annotation value to be retrieved. - /// For example, the annotation identifier for the help description is . - /// - /// An out parameter to contain the result - /// True if successful - /// - /// If the annotation value does not have a single expected type for this symbol, use the overload instead. - /// - /// Subsystem authors must use this to access annotation values, as it respects the subsystem's if it has one. - /// This value is protected because it is intended for use only by subsystem authors. It calls - /// - /// - protected internal bool TryGetAnnotation(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out TValue? value) - { - if (_annotationProvider is not null && _annotationProvider.TryGet(symbol, annotationId, out object? rawValue)) - { - if (rawValue is TValue expectedTypeValue) - { - value = expectedTypeValue; - return true; - } - throw new AnnotationTypeException(annotationId, typeof(TValue), rawValue?.GetType(), _annotationProvider); - } - - return symbol.TryGetAnnotation(annotationId, out value); - } - - /// - /// Attempt to retrieve the 's value for the annotation . This will check the - /// annotation provider that was passed to the subsystem constructor, and the internal annotation storage. - /// - /// The symbol the value is attached to - /// - /// The identifier for the annotation value to be retrieved. - /// For example, the annotation identifier for the help description is . - /// - /// An out parameter to contain the result - /// True if successful - /// - /// If the expected type of the annotation value is known, use the overload instead. - /// - /// Subsystem authors must use this to access annotation values, as it respects the subsystem's if it has one. - /// This value is protected because it is intended for use only by subsystem authors. It calls - /// - /// - protected internal bool TryGetAnnotation(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out object? value) - { - if (_annotationProvider is not null && _annotationProvider.TryGet(symbol, annotationId, out value)) - { - return true; - } - - return symbol.TryGetAnnotation(annotationId, out value); - } - /// /// The subystem executes, even if another subsystem has handled the operation. This is expected to be used in things like error reporting. /// diff --git a/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs b/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs index dd4d9e4fd5..a1bd2ede9d 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs @@ -1,6 +1,7 @@ // 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.Parsing; using System.CommandLine.Subsystems.Annotations; using System.Diagnostics.CodeAnalysis; diff --git a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs index e3dd610b38..58602e5f45 100644 --- a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs @@ -14,8 +14,8 @@ public sealed class ValidationSubsystem : CliSubsystem private Dictionary valueValidators = []; private Dictionary commandValidators = []; - private ValidationSubsystem(IAnnotationProvider? annotationProvider = null) - : base("", SubsystemKind.Validation, annotationProvider) + private ValidationSubsystem() + : base("", SubsystemKind.Validation) { } public static ValidationSubsystem Create() diff --git a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs index 1ca563b7e4..1729541d38 100644 --- a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs @@ -84,25 +84,49 @@ public static void SetValueCondition(this CliCommand symbol, /// /// The option or argument to get the conditions for. /// The conditions that have been applied to the option or argument. - /// + /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace public static List? GetValueConditions(this CliValueSymbol symbol) => symbol.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions) ? valueConditions : null; + /// + /// Gets a list of conditions on an option or argument. + /// + /// The option or argument to get the conditions for. + /// The conditions that have been applied to the option or argument. + /// + // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace + public static List? GetValueConditions(this AnnotationResolver resolver, CliValueSymbol symbol) + => resolver.TryGet>(symbol, ValueConditionAnnotations.ValueConditions, out var valueConditions) + ? valueConditions + : null; + /// /// Gets a list of conditions on a command. /// /// The command to get the conditions for. /// The conditions that have been applied to the command. - /// + /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace public static List? GetCommandConditions(this CliCommand command) => command.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions) ? valueConditions : null; + /// + /// Gets a list of conditions on a command. + /// + /// The command to get the conditions for. + /// The conditions that have been applied to the command. + /// + // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace + public static List? GetCommandConditions(this AnnotationResolver resolver, CliCommand command) + => resolver.TryGet>(command, ValueConditionAnnotations.ValueConditions, out var valueConditions) + ? valueConditions + : null; + /// /// Gets the condition that matches the type, if it exists on this option or argument. /// diff --git a/src/System.CommandLine.Subsystems/VersionSubsystem.cs b/src/System.CommandLine.Subsystems/VersionSubsystem.cs index 1aac8b75a1..c8b8c784dc 100644 --- a/src/System.CommandLine.Subsystems/VersionSubsystem.cs +++ b/src/System.CommandLine.Subsystems/VersionSubsystem.cs @@ -11,8 +11,8 @@ public class VersionSubsystem : CliSubsystem { private string? specificVersion = null; - public VersionSubsystem(IAnnotationProvider? annotationProvider = null) - : base(VersionAnnotations.Prefix, SubsystemKind.Version, annotationProvider) + public VersionSubsystem() + : base(VersionAnnotations.Prefix, SubsystemKind.Version) { } From 0914d565bfd6c1eaf0808e6c7586720a3e6a3bdf Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Wed, 18 Sep 2024 22:49:03 -0400 Subject: [PATCH 2/6] Pass additional context to annotation providers This allows more powerful dynamic resolution of annotation values --- .../AlternateSubsystems.cs | 2 +- src/System.CommandLine.Subsystems/Pipeline.cs | 6 ---- .../PipelineResult.cs | 4 +++ .../Annotations/AnnotationResolveContext.cs | 36 +++++++++++++++++++ .../Annotations/AnnotationResolver.cs | 13 ++++--- .../Annotations/IAnnotationProvider.cs | 22 ++++++++++++ .../Subsystems/IAnnotationProvider.cs | 16 --------- 7 files changed, 72 insertions(+), 27 deletions(-) create mode 100644 src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolveContext.cs create mode 100644 src/System.CommandLine.Subsystems/Subsystems/Annotations/IAnnotationProvider.cs delete mode 100644 src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs diff --git a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs index 864ec95a18..c29ee0026f 100644 --- a/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs +++ b/src/System.CommandLine.Subsystems.Tests/AlternateSubsystems.cs @@ -30,7 +30,7 @@ public VersionThatUsesHelpData(CliSymbol symbol) public override void Execute(PipelineResult pipelineResult) { - pipelineResult.Pipeline.Annotations.TryGet(Symbol, HelpAnnotations.Description, out string? description); + pipelineResult.Annotations.TryGet(Symbol, HelpAnnotations.Description, out string? description); pipelineResult.ConsoleHack.WriteLine(description); pipelineResult.AlreadyHandled = true; pipelineResult.SetSuccess(); diff --git a/src/System.CommandLine.Subsystems/Pipeline.cs b/src/System.CommandLine.Subsystems/Pipeline.cs index 5704bc45bf..0f8711efd6 100644 --- a/src/System.CommandLine.Subsystems/Pipeline.cs +++ b/src/System.CommandLine.Subsystems/Pipeline.cs @@ -75,7 +75,6 @@ private Pipeline(IEnumerable? annotationProviders = null) this.annotationProviders = annotationProviders is not null ? [..annotationProviders] : []; - Annotations = new(this.annotationProviders); } /// @@ -194,11 +193,6 @@ public ErrorReportingSubsystem? ErrorReporting /// public ResponseSubsystem Response { get; } - /// - /// Gets the annotation resolver - /// - public AnnotationResolver Annotations { get; } - /// /// Gets the list of annotation providers registered with the pipeline /// diff --git a/src/System.CommandLine.Subsystems/PipelineResult.cs b/src/System.CommandLine.Subsystems/PipelineResult.cs index 55b5f4963e..cb1763c3d1 100644 --- a/src/System.CommandLine.Subsystems/PipelineResult.cs +++ b/src/System.CommandLine.Subsystems/PipelineResult.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.CommandLine.Parsing; +using System.CommandLine.Subsystems.Annotations; namespace System.CommandLine; @@ -18,6 +19,7 @@ public PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeli Pipeline = pipeline ?? Pipeline.CreateEmpty(); ConsoleHack = consoleHack ?? new ConsoleHack(); valueProvider = new ValueProvider(this); + Annotations = new AnnotationResolver(this); } public ParseResult ParseResult { get; } @@ -27,6 +29,8 @@ public PipelineResult(ParseResult parseResult, string rawInput, Pipeline? pipeli public Pipeline Pipeline { get; } public ConsoleHack ConsoleHack { get; } + public AnnotationResolver Annotations { get; } + public bool AlreadyHandled { get; set; } public int ExitCode { get; set; } diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolveContext.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolveContext.cs new file mode 100644 index 0000000000..a13fffea1c --- /dev/null +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolveContext.cs @@ -0,0 +1,36 @@ +// 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.Parsing; + +namespace System.CommandLine.Subsystems.Annotations; + +/// +/// Additional context that is passed to . +/// +/// +/// This class exists so that additional context properties can be added without +/// breaking existing implementations. +/// +/// This is intended to be usable independently of the pipeline. For example, a method could be +/// implemented that takes a and prints help output based on the help +/// annotations in the tree, which would then be usable by developers who +/// are using the API directly. +/// +/// +public class AnnotationResolveContext(ParseResult parseResult) +{ + public AnnotationResolveContext(PipelineResult pipelineResult) + : this(pipelineResult.ParseResult) + { + } + + /// + /// The for which annotations are being resolved. + /// + /// + /// This may be used to resolve different values for an annotation based on the parents of the symbol, + /// or based on values of other symbols in the parse result. + /// + public ParseResult ParseResult { get; } = parseResult; +} diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs index 6831a75876..05c07cd5f5 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs @@ -10,13 +10,18 @@ namespace System.CommandLine.Subsystems.Annotations; /// taking into account both the values stored directly on the symbol via extension methods /// and any values from providers. /// +/// The providers from which annotation values will be resolved +/// The context for resolving annotation values /// /// The will be enumerated each time an annotation value is requested. /// It may be modified after the resolver is created. /// -public class AnnotationResolver(ICollection providers) +public class AnnotationResolver(IEnumerable providers, AnnotationResolveContext context) { - private readonly IEnumerable providers = providers ?? throw new ArgumentNullException(nameof(providers)); + public AnnotationResolver(PipelineResult pipelineResult) + : this(pipelineResult.Pipeline.AnnotationProviders, new AnnotationResolveContext(pipelineResult)) + { + } /// /// Attempt to retrieve the 's value for the annotation . This will check any @@ -48,7 +53,7 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNull { foreach (var provider in providers) { - if (provider.TryGet(symbol, annotationId, out object? rawValue)) + if (provider.TryGet(symbol, annotationId, context, out object? rawValue)) { if (rawValue is TValue expectedTypeValue) { @@ -87,7 +92,7 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(tru { foreach (var provider in providers) { - if (provider.TryGet(symbol, annotationId, out value)) + if (provider.TryGet(symbol, annotationId, context, out value)) { return true; } diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/IAnnotationProvider.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/IAnnotationProvider.cs new file mode 100644 index 0000000000..131769e9c0 --- /dev/null +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/IAnnotationProvider.cs @@ -0,0 +1,22 @@ +// 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.Diagnostics.CodeAnalysis; + +namespace System.CommandLine.Subsystems.Annotations; + +/// +/// Alternative storage of annotations, enabling lazy loading and dynamic annotations. +/// +public interface IAnnotationProvider +{ + /// + /// Try to get the value of the annotation with the given for the . + /// + /// Additional context that may be used when resolving the annotation value. + /// The symbol + /// The annotation identifier + /// The annotation value + /// if the symbol was resolved, otherwise + bool TryGet(CliSymbol symbol, AnnotationId annotationId, AnnotationResolveContext context, [NotNullWhen(true)] out object? value); +} diff --git a/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs b/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs deleted file mode 100644 index a1bd2ede9d..0000000000 --- a/src/System.CommandLine.Subsystems/Subsystems/IAnnotationProvider.cs +++ /dev/null @@ -1,16 +0,0 @@ -// 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.Parsing; -using System.CommandLine.Subsystems.Annotations; -using System.Diagnostics.CodeAnalysis; - -namespace System.CommandLine.Subsystems; - -/// -/// Alternative storage of annotations, enabling lazy loading and dynamic annotations. -/// -public interface IAnnotationProvider -{ - bool TryGet(CliSymbol symbol, AnnotationId id, [NotNullWhen(true)] out object? value); -} From 71f3ca47a78bce31b39804e05d024f52cc4242f9 Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Thu, 19 Sep 2024 01:17:15 -0400 Subject: [PATCH 3/6] Implement multivalued annotations Annotations may be defined as having multiple values, in which case multiple values may be added to the internal storage on the symbol, and providers may return enumerations of values. New annotation enumeration APIs allow enumerating the values from the symbol and the providers. --- .../AnnotationCollectionTypeException.cs | 34 ++++ .../Annotations/AnnotationResolver.cs | 75 +++++++- .../AnnotationStorageExtensions.cs | 171 ++++++++++++++---- .../Annotations/AnnotationTypeException.cs | 12 +- .../ValidationSubsystem.cs | 23 ++- .../ValueConditionAnnotationExtensions.cs | 55 ++---- 6 files changed, 269 insertions(+), 101 deletions(-) create mode 100644 src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationCollectionTypeException.cs diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationCollectionTypeException.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationCollectionTypeException.cs new file mode 100644 index 0000000000..97ba7ea921 --- /dev/null +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationCollectionTypeException.cs @@ -0,0 +1,34 @@ +// 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.Subsystems.Annotations; + +/// +/// Thrown when an annotation collection value does not match the expected type for that . +/// +public class AnnotationCollectionTypeException(AnnotationId annotationId, Type expectedType, Type? actualType, IAnnotationProvider? provider = null) + : AnnotationTypeException(annotationId, expectedType, actualType, provider) +{ + public override string Message + { + get + { + if (Provider is not null) + { + return + $"Typed accessor for annotation '${AnnotationId}' expected collection of values of type " + + $"'{ExpectedType}' but the annotation provider returned an annotation value of type " + + $"'{ActualType?.ToString() ?? "[null]"}'. " + + $"This may be an authoring error in in the annotation provider '{Provider.GetType()}' or in a " + + "typed annotation accessor."; + + } + + return + $"Typed accessor for annotation '${AnnotationId}' expected collection of values of type '{ExpectedType}' " + + $"but the stored annotation value is of type '{ActualType?.ToString() ?? "[null]"}'. " + + $"This may be an authoring error in a typed annotation accessor, or the annotation may have been stored " + + $"directly with the incorrect type, bypassing the typed accessors."; + } + } +} diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs index 05c07cd5f5..f49ce86cad 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs @@ -25,14 +25,14 @@ public AnnotationResolver(PipelineResult pipelineResult) /// /// Attempt to retrieve the 's value for the annotation . This will check any - /// annotation providers that were passed to the constructor, and the internal per-symbol annotation storage. + /// annotation providers that were passed to the resolver, and the internal per-symbol annotation storage. /// /// /// The expected type of the annotation value. If the type does not match, a will be thrown. /// If the annotation allows multiple types for its values, and a type parameter cannot be determined statically, - /// use to access the annotation value without checking its type. + /// use to access the annotation value without checking its type. /// - /// The symbol the value is attached to + /// The symbol that is annotated /// /// The identifier for the annotation value to be retrieved. /// For example, the annotation identifier for the help description is . @@ -69,9 +69,9 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNull /// /// Attempt to retrieve the 's value for the annotation . This will check any - /// annotation providers that were passed to the constructor, and the internal per-symbol annotation storage. + /// annotation providers that were passed to the resolver, and the internal per-symbol annotation storage. /// - /// The symbol the value is attached to + /// The symbol that is annotated /// /// The identifier for the annotation value to be retrieved. /// For example, the annotation identifier for the help description is . @@ -90,6 +90,13 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNull /// public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out object? value) { + // a value set directly on the symbol takes precedence over values returned by providers. + if (symbol.TryGetAnnotation(annotationId, out value)) + { + return true; + } + + // Providers are given precedence in the order they were provided to the resolver. foreach (var provider in providers) { if (provider.TryGet(symbol, annotationId, context, out value)) @@ -98,7 +105,7 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(tru } } - return symbol.TryGetAnnotation(annotationId, out value); + return false; } /// @@ -128,4 +135,60 @@ public bool TryGet(CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(tru return default; } + + /// + /// For an annotation that permits multiple values, enumerate the values associated with + /// the . If the annotation is not set, an empty enumerable will be returned. This will + /// check any annotation providers that were passed to the resolver, and the internal per-symbol annotation storage. + /// + /// + /// The expected type of the annotation value. If the type does not match, a + /// will be thrown. + /// + /// The symbol that is annotated + /// + /// The identifier for the annotation value to be retrieved. + /// For example, the annotation identifier for the help description is . + /// + /// An out parameter to contain the result + /// True if successful + /// + /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation + /// ID should also define an accessor extension method on extension method + /// on that subsystem authors can use to access the annotation value, such as + /// . + /// + public IEnumerable Enumerate(CliSymbol symbol, AnnotationId annotationId) + { + // Values added directly on the symbol take precedence over values returned by providers, + // so they are returned first. + // NOTE: EnumerateAnnotations returns these in the reverse order they were added, which means that + // callers that take the first value of a given subtype will get the most recently added value of + // that subtype that the CLI author added to the symbol. + foreach (var value in symbol.EnumerateAnnotations(annotationId)) + { + yield return value; + } + + // Providers are given precedence in the order they were provided to the resolver. + foreach (var provider in providers) + { + if (!provider.TryGet(symbol, annotationId, context, out object? rawValue)) + { + continue; + } + + if (rawValue is IEnumerable expectedTypeValue) + { + foreach (var value in expectedTypeValue) + { + yield return value; + } + } + else + { + throw new AnnotationTypeException(annotationId, typeof(IEnumerable), rawValue?.GetType(), provider); + } + } + } } \ No newline at end of file diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs index 6e2a65cf56..b62f205257 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs @@ -92,8 +92,6 @@ public static TSymbol WithAnnotation(this TSymbol symbol, AnnotationId /// /// /// The expected type of the annotation value. If the type does not match, a will be thrown. - /// If the annotation allows multiple types for its values, and a type parameter cannot be determined statically, - /// use to access the annotation value without checking its type. /// /// The symbol that is annotated /// @@ -102,10 +100,7 @@ public static TSymbol WithAnnotation(this TSymbol symbol, AnnotationId /// The annotation value, if successful, otherwise default /// True if successful /// - /// If the annotation value does not have a single expected type for this symbol, use the overload instead. - /// /// This is intended to be called by the implementation of specialized ID-specific accessors for CLI authors such as . - /// /// /// Subsystems should not call this directly, as it does not account for values from the pipeline's . /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using @@ -113,9 +108,12 @@ public static TSymbol WithAnnotation(this TSymbol symbol, AnnotationId /// extension method such as . /// /// + /// + /// Thrown when the type of the annotation value does not match the expected type. + /// public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out TValue? value) { - if (TryGetAnnotation(symbol, annotationId, out object? rawValue)) + if (symbolToAnnotationStorage.TryGetValue(symbol, out var storage) && storage.TryGet(symbol, annotationId, out var rawValue)) { if (rawValue is TValue expectedTypeValue) { @@ -132,65 +130,158 @@ public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId /// /// Attempts to get the value for the annotation associated with the /// in the internal annotation storage used to store values set via - /// . + /// . /// + /// The type of the annotation value /// The symbol that is annotated /// - /// The identifier for the annotation. For example, the annotation identifier for the help - /// description is . + /// The identifier for the annotation. For example, the annotation identifier for the help description + /// is . /// - /// The annotation value, if successful, otherwise default - /// True if successful + /// The annotation value, if successful, otherwise default /// - /// If the expected type of the annotation value is known, use the - /// overload instead. - /// - /// This is intended to be called by the implementation of specialized ID-specific accessors for - /// CLI authors such as . - /// - /// - /// Subsystems should not call this directly, as it does not account for values from the pipeline's . - /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// Subsystems should not call this directly, as it does not account for values from the pipeline's + /// . + /// They should instead access annotations from the property using /// or an ID-specific /// extension method such as . - /// /// - public static bool TryGetAnnotation(this CliSymbol symbol, AnnotationId annotationId, [NotNullWhen(true)] out object? value) + /// + /// Thrown when the type of the annotation value does not match the expected type. + /// + public static TValue? GetAnnotationOrDefault(this CliSymbol symbol, AnnotationId annotationId) { - if (symbolToAnnotationStorage.TryGetValue(symbol, out var storage) && storage.TryGet(symbol, annotationId, out value)) + if (symbol.TryGetAnnotation(annotationId, out TValue? value)) { - return true; + return value; } - value = default; - return false; + return default; } /// - /// Attempts to get the value for the annotation associated with the - /// in the internal annotation storage used to store values set via - /// . + /// For an annotation that permits multiple values, add this value to the collection + /// associated with in the internal annotation storage. /// - /// The type of the annotation value /// The symbol that is annotated - /// - /// The identifier for the annotation. For example, the annotation identifier for the help description - /// is . + /// + /// The identifier for the annotation. For example, the annotation identifier for the help description is . /// - /// The annotation value, if successful, otherwise default + /// The annotation value + public static void AddAnnotation(this CliSymbol symbol, AnnotationId annotationId, object value) + { + var storage = symbolToAnnotationStorage.GetValue(symbol, static (CliSymbol _) => new AnnotationStorage()); + if (!storage.TryGet(symbol, annotationId, out var existingValue)) + { + // avoid creation of the list until we have a second value + storage.Set(symbol, annotationId, value); + return; + } + + if (existingValue is AnnotationList existingList) + { + existingList.Add(value); + return; + } + + storage.Set(symbol, annotationId, new AnnotationList { existingValue, value }); + } + + /// + /// For an annotation that permits multiple values, attempt to remove this value from the collection + /// associated with in the internal annotation storage. + /// + /// The symbol that is annotated + /// + /// The identifier for the annotation. For example, the annotation identifier for the help description is + /// . + /// + /// The annotation value + /// True if the value was removed, false if the value was not found + public static bool RemoveAnnotation(this CliSymbol symbol, AnnotationId annotationId, object value) + { + var storage = symbolToAnnotationStorage.GetValue(symbol, static (CliSymbol _) => new AnnotationStorage()); + if (!storage.TryGet(symbol, annotationId, out var existingValue)) + { + return false; + } + + if (existingValue is not AnnotationList existingList) + { + if (Equals(existingValue, value)) + { + storage.Set(symbol, annotationId, null); + return true; + } + return false; + } + + return existingList.Remove(value); + } + + /// + /// For an annotation that permits multiple values, enumerate the values associated with + /// the . If the annotation is not set, an empty enumerable will be returned. + /// + /// The expected types of the annotation value. + /// If a value type does not match, a will be thrown. + /// The symbol that is annotated + /// + /// The identifier for the annotation. For example, the annotation identifier for the help description is + /// . + /// + /// The annotation values /// - /// Subsystems should not call this directly, as it does not account for values from the pipeline's . - /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// The values are returned in the reverse order they were added, so that the first value enumerated is the + /// last value added. This means that if callers take the first value of a given subtype, this will give the + /// most recent value of the expected type. + /// + /// This is intended to be called by the implementation of specialized ID-specific accessors for + /// CLI authors such as . + /// + /// + /// Subsystems should not call this directly, as it does not account for values from the pipeline's + /// . + /// They should instead access annotations from the property using /// or an ID-specific /// extension method such as . + /// /// - public static TValue? GetAnnotationOrDefault(this CliSymbol symbol, AnnotationId annotationId) + /// + /// Thrown when the type of the annotation value does not match the expected type. + /// + public static IEnumerable EnumerateAnnotations(this CliSymbol symbol, AnnotationId annotationId) { - if (symbol.TryGetAnnotation(annotationId, out TValue? value)) + if (!symbolToAnnotationStorage.TryGetValue(symbol, out var storage) || !storage.TryGet(symbol, annotationId, out var rawValue)) { + yield break; + } + + if (rawValue is AnnotationList list) { - return value; + // NOTE: These are returned in the reverse order they were added, which means that callers that + // take the first value of a given subtype will get the most recently added value of that subtype + // that the CLI author added to the symbol. + for(int i = list.Count - 1; i >= 0; i--) + { + if (list[i] is TValue expectedTypeValue) { + yield return expectedTypeValue; + } else { + throw new AnnotationCollectionTypeException(annotationId, typeof(TValue), rawValue?.GetType()); + } + } + } + else if (rawValue is TValue singleValue) + { + yield return singleValue; } + else + { + throw new AnnotationCollectionTypeException(annotationId, typeof(TValue), rawValue?.GetType()); + } + } - return default; + // this private subclass ensures we don't cause issues if some annotation has a expected value of type List + class AnnotationList : List + { } } diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs index 313a274ad1..1df81b3033 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationTypeException.cs @@ -20,14 +20,18 @@ public override string Message if (Provider is not null) { return - $"Typed accessor for annotation '${AnnotationId}' expected type '{ExpectedType}' but the annotation provider returned an annotation of type '{ActualType?.ToString() ?? "[null]"}'. " + - $"This may be an authoring error in in the annotation provider '{Provider.GetType()}' or in a typed annotation accessor."; + $"Typed accessor for annotation '${AnnotationId}' expected value of type '{ExpectedType}' but the " + + $"annotation provider returned an value of type '{ActualType?.ToString() ?? "[null]"}'. " + + $"This may be an authoring error in in the annotation provider '{Provider.GetType()}' or in a " + + "typed annotation accessor."; } return - $"Typed accessor for annotation '${AnnotationId}' expected type '{ExpectedType}' but the stored annotation is of type '{ActualType?.ToString() ?? "[null]"}'. " + - $"This may be an authoring error in a typed annotation accessor, or the annotation may have be stored directly with the incorrect type, bypassing the typed accessors."; + $"Typed accessor for annotation '${AnnotationId}' expected value of type '{ExpectedType}' but the stored " + + $"annotation value is of type '{ActualType?.ToString() ?? "[null]"}'. " + + $"This may be an authoring error in a typed annotation accessor, or the annotation may have be stored directly " + + $"with the incorrect type, bypassing the typed accessors."; } } } diff --git a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs index 58602e5f45..5d8d50d21f 100644 --- a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs @@ -68,27 +68,26 @@ public override void Execute(PipelineResult pipelineResult) private void ValidateValue(CliValueSymbol valueSymbol, ValidationContext validationContext) { - var valueConditions = valueSymbol.GetValueConditions(); - if (valueConditions is null) + var valueConditions = valueSymbol.EnumerateValueConditions(); + + var enumerator = valueConditions.GetEnumerator(); + if (!enumerator.MoveNext()) { - return; // nothing to do + // avoid getting the value if there are no conditions + return; } var value = validationContext.GetValue(valueSymbol); var valueResult = validationContext.GetValueResult(valueSymbol); - foreach (var condition in valueConditions) - { - ValidateValueCondition(value, valueSymbol, valueResult, condition, validationContext); - } + + do { + ValidateValueCondition(value, valueSymbol, valueResult, enumerator.Current, validationContext); + } while (enumerator.MoveNext()); } private void ValidateCommand(CliCommandResult commandResult, ValidationContext validationContext) { - var valueConditions = commandResult.Command.GetCommandConditions(); - if (valueConditions is null) - { - return; // nothing to do - } + var valueConditions = commandResult.Command.EnumerateCommandConditions(); foreach (var condition in valueConditions) { diff --git a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs index 1729541d38..f641d08890 100644 --- a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs @@ -1,4 +1,7 @@ -using System.CommandLine.Subsystems.Annotations; +// 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.ValueConditions; using System.CommandLine.ValueSources; @@ -58,26 +61,12 @@ public static void SetInclusiveGroup(this CliCommand command, IEnumerable(this TValueSymbol symbol, TValueCondition valueCondition) where TValueSymbol : CliValueSymbol where TValueCondition : ValueCondition - { - if (!symbol.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions)) - { - valueConditions = []; - symbol.SetAnnotation(ValueConditionAnnotations.ValueConditions, valueConditions); - } - valueConditions.Add(valueCondition); - } + => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, valueCondition); // TODO: This should not be public if ValueConditions are not public public static void SetValueCondition(this CliCommand symbol, TCommandCondition commandCondition) where TCommandCondition : CommandCondition - { - if (!symbol.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions)) - { - valueConditions = []; - symbol.SetAnnotation(ValueConditionAnnotations.ValueConditions, valueConditions); - } - valueConditions.Add(commandCondition); - } + => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, commandCondition); /// /// Gets a list of conditions on an option or argument. @@ -86,10 +75,8 @@ public static void SetValueCondition(this CliCommand symbol, /// The conditions that have been applied to the option or argument. /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace - public static List? GetValueConditions(this CliValueSymbol symbol) - => symbol.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions) - ? valueConditions - : null; + public static IEnumerable EnumerateValueConditions(this CliValueSymbol symbol) + => symbol.EnumerateAnnotations(ValueConditionAnnotations.ValueConditions); /// /// Gets a list of conditions on an option or argument. @@ -98,10 +85,8 @@ public static void SetValueCondition(this CliCommand symbol, /// The conditions that have been applied to the option or argument. /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace - public static List? GetValueConditions(this AnnotationResolver resolver, CliValueSymbol symbol) - => resolver.TryGet>(symbol, ValueConditionAnnotations.ValueConditions, out var valueConditions) - ? valueConditions - : null; + public static IEnumerable EnumerateValueConditions(this AnnotationResolver resolver, CliValueSymbol symbol) + => resolver.Enumerate(symbol, ValueConditionAnnotations.ValueConditions); /// /// Gets a list of conditions on a command. @@ -110,10 +95,8 @@ public static void SetValueCondition(this CliCommand symbol, /// The conditions that have been applied to the command. /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace - public static List? GetCommandConditions(this CliCommand command) - => command.TryGetAnnotation>(ValueConditionAnnotations.ValueConditions, out var valueConditions) - ? valueConditions - : null; + public static IEnumerable EnumerateCommandConditions(this CliCommand command) + => command.EnumerateAnnotations(ValueConditionAnnotations.ValueConditions); /// /// Gets a list of conditions on a command. @@ -122,10 +105,8 @@ public static void SetValueCondition(this CliCommand symbol, /// The conditions that have been applied to the command. /// // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace - public static List? GetCommandConditions(this AnnotationResolver resolver, CliCommand command) - => resolver.TryGet>(command, ValueConditionAnnotations.ValueConditions, out var valueConditions) - ? valueConditions - : null; + public static IEnumerable EnumerateCommandConditions(this AnnotationResolver resolver, CliCommand command) + => resolver.Enumerate(command, ValueConditionAnnotations.ValueConditions); /// /// Gets the condition that matches the type, if it exists on this option or argument. @@ -137,9 +118,7 @@ public static void SetValueCondition(this CliCommand symbol, // TODO: Consider removing user facing naming, other than the base type, that is Value or CommandCondition and just use Condition public static TCondition? GetValueCondition(this CliValueSymbol symbol) where TCondition : ValueCondition - => !symbol.TryGetAnnotation(ValueConditionAnnotations.ValueConditions, out List? valueConditions) - ? null - : valueConditions.OfType().LastOrDefault(); + => symbol.EnumerateValueConditions().OfType().LastOrDefault(); /// /// Gets the condition that matches the type, if it exists on this command. @@ -150,9 +129,7 @@ public static void SetValueCondition(this CliCommand symbol, // This method feels useful because it clarifies that last should win and returns one, when only one should be applied public static TCondition? GetCommandCondition(this CliCommand symbol) where TCondition : CommandCondition - => !symbol.TryGetAnnotation(ValueConditionAnnotations.ValueConditions, out List? valueConditions) - ? null - : valueConditions.OfType().LastOrDefault(); + => symbol.EnumerateCommandConditions().OfType().FirstOrDefault(); } From b5e7d057a28f4f1c4fc5d62dae6c708190f3d15a Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Thu, 19 Sep 2024 01:40:31 -0400 Subject: [PATCH 4/6] Strongly type SetRange, add GetRange --- .../ValidationSubsystemTests.cs | 30 ++++++++--------- .../ValueConditionAnnotationExtensions.cs | 33 ++++++++++++------- src/System.CommandLine/CliArgument{T}.cs | 4 +-- src/System.CommandLine/CliOption{T}.cs | 2 +- src/System.CommandLine/ICliValueSymbol.cs | 14 ++++++++ .../System.CommandLine.csproj | 1 + 6 files changed, 55 insertions(+), 29 deletions(-) create mode 100644 src/System.CommandLine/ICliValueSymbol.cs diff --git a/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs b/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs index 3ca4e362c4..91e5622241 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs @@ -11,18 +11,18 @@ 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 lowerBound, T upperBound) + private CliOption GetOptionWithSimpleRange(string name, T lowerBound, T upperBound) where T : IComparable { - var option = new CliOption("--intOpt"); + var option = new CliOption(name); option.SetRange(lowerBound, upperBound); return option; } - private CliOption GetOptionWithRangeBounds(ValueSource lowerBound, ValueSource upperBound) + private CliOption GetOptionWithRangeBounds(string name, ValueSource lowerBound, ValueSource upperBound) where T : IComparable { - var option = new CliOption("--intOpt"); + var option = new CliOption(name); option.SetRange(lowerBound, upperBound); return option; } @@ -45,7 +45,7 @@ private PipelineResult ExecutedPipelineResultForCommand(CliCommand command, stri [Fact] public void Int_values_in_specified_range_do_not_have_errors() { - var option = GetOptionWithSimpleRange(0, 50); + var option = GetOptionWithSimpleRange("--intOpt", 0, 50); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -56,7 +56,7 @@ public void Int_values_in_specified_range_do_not_have_errors() [Fact] public void Int_values_above_upper_bound_report_error() { - var option = GetOptionWithSimpleRange(0, 5); + var option = GetOptionWithSimpleRange("--intOpt", 0, 5); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -69,7 +69,7 @@ public void Int_values_above_upper_bound_report_error() [Fact] public void Int_below_lower_bound_report_error() { - var option = GetOptionWithSimpleRange(0, 5); + var option = GetOptionWithSimpleRange("--intOpt", 0, 5); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt -42"); @@ -82,7 +82,7 @@ public void Int_below_lower_bound_report_error() [Fact] public void Int_values_on_lower_range_bound_do_not_report_error() { - var option = GetOptionWithSimpleRange(42, 50); + var option = GetOptionWithSimpleRange("--intOpt", 42, 50); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -93,7 +93,7 @@ public void Int_values_on_lower_range_bound_do_not_report_error() [Fact] public void Int_values_on_upper_range_bound_do_not_report_error() { - var option = GetOptionWithSimpleRange(0, 42); + var option = GetOptionWithSimpleRange("--intOpt", 0, 42); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -104,7 +104,7 @@ public void Int_values_on_upper_range_bound_do_not_report_error() [Fact] public void Values_below_calculated_lower_bound_report_error() { - var option = GetOptionWithRangeBounds(ValueSource.Create(() => (true, 1)), 50); + var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(() => (true, 1)), 50); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 0"); @@ -118,7 +118,7 @@ public void Values_below_calculated_lower_bound_report_error() [Fact] public void Values_within_calculated_range_do_not_report_error() { - var option = GetOptionWithRangeBounds(ValueSource.Create(() => (true, 1)), ValueSource.Create(() => (true, 50))); + var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(() => (true, 1)), ValueSource.Create(() => (true, 50))); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -129,7 +129,7 @@ public void Values_within_calculated_range_do_not_report_error() [Fact] public void Values_above_calculated_upper_bound_report_error() { - var option = GetOptionWithRangeBounds(0, ValueSource.Create(() => (true, 40))); + var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create(() => (true, 40))); var pipelineResult = ExecutedPipelineResultForRangeOption(option, "--intOpt 42"); @@ -143,7 +143,7 @@ public void Values_above_calculated_upper_bound_report_error() public void Values_below_relative_lower_bound_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds(ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50); + var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 0 -a 0"); @@ -159,7 +159,7 @@ public void Values_below_relative_lower_bound_report_error() public void Values_within_relative_range_do_not_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds(ValueSource.Create(otherOption, o => (true, (int)o + 1)), ValueSource.Create(otherOption, o => (true, (int)o + 10))); + var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), ValueSource.Create(otherOption, o => (true, (int)o + 10))); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 11 -a 3"); @@ -172,7 +172,7 @@ public void Values_within_relative_range_do_not_report_error() public void Values_above_relative_upper_bound_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds(0, ValueSource.Create(otherOption, o => (true, (int)o + 10))); + var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create(otherOption, o => (true, (int)o + 10))); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 9 -a -2"); diff --git a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs index f641d08890..dd07228572 100644 --- a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs @@ -13,41 +13,52 @@ namespace System.CommandLine; public static class ValueConditionAnnotationExtensions { /// - /// Set the upper and/or lower bound values of the range. + /// Set upper and/or lower bounds on the range of values that the symbol value may have. /// - /// The type of the bounds. + /// The type of the symbol whose value is bounded by the range. + /// The type of the value that is bounded by the range. /// The option or argument the range applies to. /// The lower bound of the range. /// The upper bound of the range. // TODO: Add RangeBounds // TODO: You should not have to set both...why not nullable? - public static void SetRange(this CliValueSymbol symbol, T lowerBound, T upperBound) - where T : IComparable + public static void SetRange(this TValueSymbol symbol, TValue lowerBound, TValue upperBound) + where TValueSymbol : CliValueSymbol, ICliValueSymbol + where TValue : IComparable { - var range = new Range(lowerBound, upperBound); + var range = new Range(lowerBound, upperBound); symbol.SetValueCondition(range); } /// - /// Set the upper and/or lower bound via ValueSource. Implicit conversions means this - /// generally just works with any . + /// Set upper and/or lower bounds on the range of values that the symbol value may have. + /// Implicit conversions means this generally just works with any . /// - /// The type of the bounds. + /// The type of the symbol whose value is bounded by the range. + /// The type of the value that is bounded by the range. /// The option or argument the range applies to. /// The that is the lower bound of the range. /// The that is the upper bound of the range. // TODO: Add RangeBounds // TODO: You should not have to set both...why not nullable? - public static void SetRange(this CliValueSymbol symbol, ValueSource lowerBound, ValueSource upperBound) - where T : IComparable + public static void SetRange(this TValueSymbol symbol, ValueSource lowerBound, ValueSource upperBound) + where TValueSymbol : CliValueSymbol, ICliValueSymbol + where TValue : IComparable // TODO: You should not have to set both...why not nullable? { - var range = new Range(lowerBound, upperBound); + var range = new Range(lowerBound, upperBound); symbol.SetValueCondition(range); } + /// + /// Get the upper and/or lower bound of the symbol's value. + /// + /// The option or argument the range applies to. + public static ValueConditions.Range? GetRange(this CliValueSymbol symbol) + => symbol.GetValueCondition(); + /// /// Indicates that there is an inclusive group of options and arguments for the command. All /// members of an inclusive must be present, or none can be present. diff --git a/src/System.CommandLine/CliArgument{T}.cs b/src/System.CommandLine/CliArgument{T}.cs index 14015b7ac5..f21c2db0d5 100644 --- a/src/System.CommandLine/CliArgument{T}.cs +++ b/src/System.CommandLine/CliArgument{T}.cs @@ -4,12 +4,12 @@ using System.Collections.Generic; using System.CommandLine.Parsing; using System.Diagnostics.CodeAnalysis; -using System.IO; namespace System.CommandLine { + /// - public class CliArgument : CliArgument + public class CliArgument : CliArgument, ICliValueSymbol { // TODO: custom parser /* diff --git a/src/System.CommandLine/CliOption{T}.cs b/src/System.CommandLine/CliOption{T}.cs index f2ca874d1a..21db11cb97 100644 --- a/src/System.CommandLine/CliOption{T}.cs +++ b/src/System.CommandLine/CliOption{T}.cs @@ -7,7 +7,7 @@ namespace System.CommandLine { /// /// The that the option's arguments are expected to be parsed as. - public class CliOption : CliOption + public class CliOption : CliOption, ICliValueSymbol { // TODO: do not expose private fields internal readonly CliArgument _argument; diff --git a/src/System.CommandLine/ICliValueSymbol.cs b/src/System.CommandLine/ICliValueSymbol.cs new file mode 100644 index 0000000000..d59c3b1975 --- /dev/null +++ b/src/System.CommandLine/ICliValueSymbol.cs @@ -0,0 +1,14 @@ +// 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; + +/// +/// This is applied to and , and allows +/// allows methods with a argument to apply constraints based on the +/// value type. +/// +/// The value type +public interface ICliValueSymbol +{ +} diff --git a/src/System.CommandLine/System.CommandLine.csproj b/src/System.CommandLine/System.CommandLine.csproj index cfb211a276..bee122292b 100644 --- a/src/System.CommandLine/System.CommandLine.csproj +++ b/src/System.CommandLine/System.CommandLine.csproj @@ -27,6 +27,7 @@ + From 1c290f62d456ebd9fd8f1651ff21903d841a0cd5 Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Fri, 20 Sep 2024 17:43:22 -0400 Subject: [PATCH 5/6] Use the annotation resolver in ValueProvider --- .../ValueAnnotationExtensions.cs | 9 ++++++--- src/System.CommandLine.Subsystems/ValueProvider.cs | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs index 676bcc9b7d..68d3d6324e 100644 --- a/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueAnnotationExtensions.cs @@ -17,9 +17,12 @@ public static class ValueAnnotationExtensions /// 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 . + /// This is intended to be called by CLI authors inspecting the default value source they have + /// associated directly with the . + /// + /// Subsystems should instead use , which caches calculated + /// values and respects dynamic/lazy annotations from the . + /// /// public static bool TryGetDefaultValueSource(this CliValueSymbol valueSymbol, [NotNullWhen(true)] out ValueSource? defaultValueSource) => valueSymbol.TryGetAnnotation(ValueAnnotations.DefaultValueSource, out defaultValueSource); diff --git a/src/System.CommandLine.Subsystems/ValueProvider.cs b/src/System.CommandLine.Subsystems/ValueProvider.cs index f51a1edb63..adb6b9c90e 100644 --- a/src/System.CommandLine.Subsystems/ValueProvider.cs +++ b/src/System.CommandLine.Subsystems/ValueProvider.cs @@ -1,6 +1,7 @@ // 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; namespace System.CommandLine; @@ -48,7 +49,7 @@ private bool TryGetValue(CliSymbol symbol, out T? value) { return UseValue(valueSymbol, valueResult.GetValue()); } - if (valueSymbol.TryGetDefaultValueSource(out ValueSource? defaultValueSource)) + if (pipelineResult.Annotations.TryGet(valueSymbol, ValueAnnotations.DefaultValueSource, out ValueSource? defaultValueSource)) { if (defaultValueSource is not ValueSource typedDefaultValueSource) { From ae8534928dd4c44ab22ab39afd79fe854ea56700 Mon Sep 17 00:00:00 2001 From: Mikayla Hutchinson Date: Tue, 24 Sep 2024 15:35:29 -0400 Subject: [PATCH 6/6] Address PR feedback --- .../ValidationSubsystemTests.cs | 8 ++++--- .../Annotations/AnnotationResolver.cs | 4 ++-- .../AnnotationStorageExtensions.cs | 2 +- .../ValidationSubsystem.cs | 6 +++++- .../ValueConditionAnnotationExtensions.cs | 21 +++---------------- .../RelativeToSymbolValueSource.cs | 4 ++-- .../ValueSources/ValueSource.cs | 2 +- src/System.CommandLine/ICliValueSymbol.cs | 2 +- 8 files changed, 20 insertions(+), 29 deletions(-) diff --git a/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs b/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs index 91e5622241..e9d2e30833 100644 --- a/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs +++ b/src/System.CommandLine.Subsystems.Tests/ValidationSubsystemTests.cs @@ -143,7 +143,7 @@ public void Values_above_calculated_upper_bound_report_error() public void Values_below_relative_lower_bound_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), 50); + var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, o + 1)), 50); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 0 -a 0"); @@ -159,7 +159,9 @@ public void Values_below_relative_lower_bound_report_error() public void Values_within_relative_range_do_not_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds("--intOpt", ValueSource.Create(otherOption, o => (true, (int)o + 1)), ValueSource.Create(otherOption, o => (true, (int)o + 10))); + var option = GetOptionWithRangeBounds("--intOpt", + ValueSource.Create(otherOption, o => (true, o + 1)), + ValueSource.Create(otherOption, o => (true, o + 10))); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 11 -a 3"); @@ -172,7 +174,7 @@ public void Values_within_relative_range_do_not_report_error() public void Values_above_relative_upper_bound_report_error() { var otherOption = new CliOption("-a"); - var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create(otherOption, o => (true, (int)o + 10))); + var option = GetOptionWithRangeBounds("--intOpt", 0, ValueSource.Create(otherOption, o => (true, o + 10))); var command = new CliCommand("cmd") { option, otherOption }; var pipelineResult = ExecutedPipelineResultForCommand(command, "--intOpt 9 -a -2"); diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs index f49ce86cad..0a16cd40a1 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationResolver.cs @@ -41,8 +41,8 @@ public AnnotationResolver(PipelineResult pipelineResult) /// True if successful /// /// This is intended for use by developers defining custom annotation IDs. Anyone defining an annotation - /// ID should also define an accessor extension method on extension method - /// on that subsystem authors can use to access the annotation value, such as + /// ID should also define an accessor extension method on + /// that subsystem authors can use to access the annotation value, such as /// . /// /// If the annotation value does not have a single expected type for this symbol, use the diff --git a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs index b62f205257..cddca63bdd 100644 --- a/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs +++ b/src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs @@ -103,7 +103,7 @@ public static TSymbol WithAnnotation(this TSymbol symbol, AnnotationId /// This is intended to be called by the implementation of specialized ID-specific accessors for CLI authors such as . /// /// Subsystems should not call this directly, as it does not account for values from the pipeline's . - /// They should instead access annotations from the see cref="Pipeline.Annotations"/> property using + /// They should instead access annotations from the property using /// or an ID-specific /// extension method such as . /// diff --git a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs index 5d8d50d21f..00e44e9971 100644 --- a/src/System.CommandLine.Subsystems/ValidationSubsystem.cs +++ b/src/System.CommandLine.Subsystems/ValidationSubsystem.cs @@ -70,10 +70,11 @@ private void ValidateValue(CliValueSymbol valueSymbol, ValidationContext validat { var valueConditions = valueSymbol.EnumerateValueConditions(); + // manually implement the foreach so we can efficiently skip getting the + // value if there are no conditions var enumerator = valueConditions.GetEnumerator(); if (!enumerator.MoveNext()) { - // avoid getting the value if there are no conditions return; } @@ -89,6 +90,9 @@ private void ValidateCommand(CliCommandResult commandResult, ValidationContext v { var valueConditions = commandResult.Command.EnumerateCommandConditions(); + // unlike ValidateValue we do not need to manually implement the foreach + // to skip unnecessary work, as there is no additional work to be before + // calling command validators foreach (var condition in valueConditions) { ValidateCommandCondition(commandResult, condition, validationContext); diff --git a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs index dd07228572..77a9e65b8c 100644 --- a/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs +++ b/src/System.CommandLine.Subsystems/ValueConditionAnnotationExtensions.cs @@ -20,8 +20,7 @@ public static class ValueConditionAnnotationExtensions /// The option or argument the range applies to. /// The lower bound of the range. /// The upper bound of the range. - // TODO: Add RangeBounds - // TODO: You should not have to set both...why not nullable? + // TODO: can we eliminate this overload and just reply on the implicit cast to ValueSource? public static void SetRange(this TValueSymbol symbol, TValue lowerBound, TValue upperBound) where TValueSymbol : CliValueSymbol, ICliValueSymbol where TValue : IComparable @@ -40,12 +39,9 @@ public static void SetRange(this TValueSymbol symbol, TVal /// The option or argument the range applies to. /// The that is the lower bound of the range. /// The that is the upper bound of the range. - // TODO: Add RangeBounds - // TODO: You should not have to set both...why not nullable? - public static void SetRange(this TValueSymbol symbol, ValueSource lowerBound, ValueSource upperBound) + public static void SetRange(this TValueSymbol symbol, ValueSource? lowerBound, ValueSource? upperBound) where TValueSymbol : CliValueSymbol, ICliValueSymbol where TValue : IComparable - // TODO: You should not have to set both...why not nullable? { var range = new Range(lowerBound, upperBound); @@ -68,13 +64,11 @@ public static void SetRange(this TValueSymbol symbol, Valu public static void SetInclusiveGroup(this CliCommand command, IEnumerable group) => command.SetValueCondition(new InclusiveGroup(group)); - // TODO: This should not be public if ValueConditions are not public public static void SetValueCondition(this TValueSymbol symbol, TValueCondition valueCondition) where TValueSymbol : CliValueSymbol where TValueCondition : ValueCondition => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, valueCondition); - // TODO: This should not be public if ValueConditions are not public public static void SetValueCondition(this CliCommand symbol, TCommandCondition commandCondition) where TCommandCondition : CommandCondition => symbol.AddAnnotation(ValueConditionAnnotations.ValueConditions, commandCondition); @@ -84,8 +78,6 @@ public static void SetValueCondition(this CliCommand symbol, /// /// The option or argument to get the conditions for. /// The conditions that have been applied to the option or argument. - /// - // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace public static IEnumerable EnumerateValueConditions(this CliValueSymbol symbol) => symbol.EnumerateAnnotations(ValueConditionAnnotations.ValueConditions); @@ -94,8 +86,6 @@ public static IEnumerable EnumerateValueConditions(this CliValue /// /// The option or argument to get the conditions for. /// The conditions that have been applied to the option or argument. - /// - // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace public static IEnumerable EnumerateValueConditions(this AnnotationResolver resolver, CliValueSymbol symbol) => resolver.Enumerate(symbol, ValueConditionAnnotations.ValueConditions); @@ -104,8 +94,6 @@ public static IEnumerable EnumerateValueConditions(this Annotati /// /// The command to get the conditions for. /// The conditions that have been applied to the command. - /// - // TODO: This is public because it will be used by other subsystems we might not own. It could be an extension method the subsystem namespace public static IEnumerable EnumerateCommandConditions(this CliCommand command) => command.EnumerateAnnotations(ValueConditionAnnotations.ValueConditions); @@ -125,11 +113,9 @@ public static IEnumerable EnumerateCommandConditions(this Anno /// The type of condition to return. /// The option or argument that may contain the condition. /// The condition if it exists on the option or argument, otherwise null. - // This method feels useful because it clarifies that last should win and returns one, when only one should be applied - // TODO: Consider removing user facing naming, other than the base type, that is Value or CommandCondition and just use Condition public static TCondition? GetValueCondition(this CliValueSymbol symbol) where TCondition : ValueCondition - => symbol.EnumerateValueConditions().OfType().LastOrDefault(); + => symbol.EnumerateValueConditions().OfType().FirstOrDefault(); /// /// Gets the condition that matches the type, if it exists on this command. @@ -137,7 +123,6 @@ public static IEnumerable EnumerateCommandConditions(this Anno /// The type of condition to return. /// The command that may contain the condition. /// The condition if it exists on the command, otherwise null. - // This method feels useful because it clarifies that last should win and returns one, when only one should be applied public static TCondition? GetCommandCondition(this CliCommand symbol) where TCondition : CommandCondition => symbol.EnumerateCommandConditions().OfType().FirstOrDefault(); diff --git a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs index a53e13c573..ba48870130 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/RelativeToSymbolValueSource.cs @@ -17,7 +17,7 @@ public sealed class RelativeToSymbolValueSource internal RelativeToSymbolValueSource( CliValueSymbol otherSymbol, bool onlyUserEnteredValues = false, - Func? calculation = null, + Func? calculation = null, string? description = null) { OtherSymbol = otherSymbol; @@ -29,7 +29,7 @@ internal RelativeToSymbolValueSource( public override string? Description { get; } public CliValueSymbol OtherSymbol { get; } public bool OnlyUserEnteredValues { get; } - public Func? Calculation { get; } + public Func? Calculation { get; } /// public override bool TryGetTypedValue(PipelineResult pipelineResult, out T? value) diff --git a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs index 43098896ed..18ef7a97b6 100644 --- a/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs +++ b/src/System.CommandLine.Subsystems/ValueSources/ValueSource.cs @@ -31,7 +31,7 @@ public static ValueSource Create(Func<(bool success, T? value)> calculatio => new CalculatedValueSource(calculation, description); public static ValueSource Create(CliValueSymbol otherSymbol, - Func? calculation = null, + Func? calculation = null, bool userEnteredValueOnly = false, string? description = null) => new RelativeToSymbolValueSource(otherSymbol, userEnteredValueOnly, calculation, description); diff --git a/src/System.CommandLine/ICliValueSymbol.cs b/src/System.CommandLine/ICliValueSymbol.cs index d59c3b1975..d4d7dbdddf 100644 --- a/src/System.CommandLine/ICliValueSymbol.cs +++ b/src/System.CommandLine/ICliValueSymbol.cs @@ -4,7 +4,7 @@ namespace System.CommandLine; /// -/// This is applied to and , and allows +/// This is implemented only by and , and allows /// allows methods with a argument to apply constraints based on the /// value type. ///