diff --git a/src/Repl.Core/Help/HelpTextBuilder.Rendering.cs b/src/Repl.Core/Help/HelpTextBuilder.Rendering.cs index 35ff2b3..caa47fd 100644 --- a/src/Repl.Core/Help/HelpTextBuilder.Rendering.cs +++ b/src/Repl.Core/Help/HelpTextBuilder.Rendering.cs @@ -208,7 +208,7 @@ or OptionSchemaTokenKind.ValueAlias parameterType = groupInfo.Property.PropertyType; description = groupInfo.Property.GetCustomAttribute()?.Description ?? string.Empty; var propDefault = groupInfo.Property.GetValue(groupInfo.DefaultInstance); - defaultValue = propDefault is not null && !IsDefaultForType(propDefault, parameterType) + defaultValue = propDefault is not null && !ParsingOptions.IsDefaultForType(propDefault, parameterType) ? $" [default: {propDefault}]" : string.Empty; } @@ -314,31 +314,6 @@ private static Type UnwrapAsyncReturnType(Type returnType) : returnType; } - private static bool IsDefaultForType(object value, Type type) - { - if (type == typeof(bool)) - { - return value is false; - } - - if (type == typeof(int)) - { - return value is 0; - } - - if (type == typeof(long)) - { - return value is 0L; - } - - if (type == typeof(double)) - { - return value is 0.0d; - } - - return false; - } - private static string ResolveOptionPlaceholder(Type parameterType) { var effectiveType = Nullable.GetUnderlyingType(parameterType) ?? parameterType; @@ -619,13 +594,17 @@ private static string[][] BuildGlobalOptionRows(ParsingOptions parsingOptions) .OrderBy(option => option.Name, StringComparer.OrdinalIgnoreCase) .Select(option => { - var aliases = option.Aliases.Count == 0 - ? string.Empty - : $", {string.Join(", ", option.Aliases)}"; + var aliases = option.Aliases.Count == 0 + ? string.Empty + : $", {string.Join(", ", option.Aliases)}"; + var description = string.IsNullOrWhiteSpace(option.Description) + ? "Custom global option." + : option.Description; + return new[] { $"{option.CanonicalToken}{aliases}", - "Custom global option.", + description, }; }); return [.. BuiltInGlobalOptionRows.Concat(customRows)]; diff --git a/src/Repl.Core/Parsing/GlobalOptionDefinition.cs b/src/Repl.Core/Parsing/GlobalOptionDefinition.cs index fd79451..6fd93bc 100644 --- a/src/Repl.Core/Parsing/GlobalOptionDefinition.cs +++ b/src/Repl.Core/Parsing/GlobalOptionDefinition.cs @@ -5,5 +5,6 @@ internal sealed record GlobalOptionDefinition( string CanonicalToken, IReadOnlyList Aliases, string? DefaultValue, + string? Description, Type ValueType, Type? OwnerType); diff --git a/src/Repl.Core/ParsingOptions.cs b/src/Repl.Core/ParsingOptions.cs index b0b25c5..1b4084b 100644 --- a/src/Repl.Core/ParsingOptions.cs +++ b/src/Repl.Core/ParsingOptions.cs @@ -1,3 +1,4 @@ +using System.Diagnostics.CodeAnalysis; using System.Globalization; namespace Repl; @@ -101,7 +102,25 @@ internal bool TryGetRouteConstraint(string name, out Func predicat /// Optional aliases. Values without prefix are normalized to --alias. /// Optional default value metadata. public void AddGlobalOption(string name, string[]? aliases = null, T? defaultValue = default) => - AddGlobalOptionCore(name, typeof(T), aliases, defaultValue?.ToString()); + AddGlobalOptionCore(name, typeof(T), aliases, FormatDefaultValue(defaultValue, typeof(T))); + + /// + /// Registers a custom global option with an explicit help description. + /// + /// + /// is the trailing required parameter so this overload never collides with + /// during overload resolution: a positional + /// null second argument (for example AddGlobalOption<string>("tenant", null)) binds + /// unambiguously to the aliases-only overload. The typed + /// path (UseGlobalOptions<T>()) remains the primary way to attach descriptions. + /// + /// Declared value type. + /// Canonical name without prefix (for example: "tenant"). + /// Aliases (pass null for none). Values without prefix are normalized to --alias. + /// Default value metadata (pass default for none). + /// Description shown in help output. + public void AddGlobalOption(string name, string[]? aliases, T? defaultValue, string description) => + AddGlobalOptionCore(name, typeof(T), aliases, FormatDefaultValue(defaultValue, typeof(T)), description); /// /// Registers a custom global option using a type or constraint name @@ -117,7 +136,27 @@ public void AddGlobalOption(string name, string[]? aliases = null, T? default public void AddGlobalOption(string name, string constraintOrTypeName, string[]? aliases = null, string? defaultValue = null) => AddGlobalOptionCore(name, ResolveConstraintOrTypeName(constraintOrTypeName, _customRouteConstraints), aliases, defaultValue); - internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases, string? defaultValue, Type? ownerType = null) + /// + /// Registers a custom global option (by type or constraint name) with an explicit help description. + /// + /// + /// is the trailing required parameter so this overload never collides with + /// during overload resolution: a positional + /// null third argument (for example AddGlobalOption("port", "int", null)) binds unambiguously + /// to the aliases-only overload. + /// + /// Canonical name without prefix (for example: "tenant"). + /// + /// Built-in type name ("string", "int", "long", "bool", "guid", "uri", "date", "datetime", "timespan") + /// or a registered custom route constraint name. Custom constraints resolve to string. + /// + /// Aliases (pass null for none). Values without prefix are normalized to --alias. + /// Default value as string (pass null for none). + /// Description shown in help output. + public void AddGlobalOption(string name, string constraintOrTypeName, string[]? aliases, string? defaultValue, string description) => + AddGlobalOptionCore(name, ResolveConstraintOrTypeName(constraintOrTypeName, _customRouteConstraints), aliases, defaultValue, description); + + internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases, string? defaultValue, string? description = null, Type? ownerType = null) { name = string.IsNullOrWhiteSpace(name) ? throw new ArgumentException("Global option name cannot be empty.", nameof(name)) @@ -141,6 +180,7 @@ internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases CanonicalToken: normalizedCanonical, Aliases: normalizedAliases, DefaultValue: defaultValue, + Description: description, ValueType: valueType, OwnerType: ownerType); } @@ -161,6 +201,34 @@ private static string BuildDuplicateGlobalOptionMessage(string name, Type? exist return $"A global option named '{name}' is already registered by {existingSource} and cannot also be registered by {newSource}."; } + internal static string? FormatDefaultValue(object? value, Type type) => + value is not null && !IsDefaultForType(value, type) + ? value.ToString() + : null; + + [UnconditionalSuppressMessage( + "Trimming", + "IL2067", + Justification = "Activator.CreateInstance is only reached for value types, which always have a parameterless constructor.")] + internal static bool IsDefaultForType(object value, Type type) + { + // The default of a Nullable is null, never a value: a nullable property or + // parameter initialized to 0 or false is a deliberate default that must be kept + // (rendered in help, applied at resolution), unlike the implicit default of the + // underlying non-nullable type. + if (Nullable.GetUnderlyingType(type) is not null) + { + return false; + } + + // Any non-nullable value type compares against its boxed CLR default (false, 0, + // Guid.Empty, enum zero, DateTime.MinValue, ...), so implicit defaults captured + // by `T? defaultValue = default` never become registration metadata. Reference + // types have no implicit non-null default to suppress. Registration-time only, + // so the boxing allocation is acceptable. + return type.IsValueType && value.Equals(Activator.CreateInstance(type)); + } + private static Type ResolveConstraintOrTypeName( string constraintOrTypeName, Dictionary> customConstraints) diff --git a/src/Repl.Defaults/GlobalOptionsExtensions.cs b/src/Repl.Defaults/GlobalOptionsExtensions.cs index af3c1f6..d19d8c6 100644 --- a/src/Repl.Defaults/GlobalOptionsExtensions.cs +++ b/src/Repl.Defaults/GlobalOptionsExtensions.cs @@ -34,9 +34,14 @@ public static class GlobalOptionsExtensions var optionAttr = property.GetCustomAttribute(); var name = optionAttr?.Name ?? ToKebabCase(property.Name); var aliases = optionAttr?.Aliases; + // The effective default of a typed global option is always the prototype value: + // PopulateInstance starts from new T() and only overwrites parsed values. Keep + // the metadata aligned (even when the value equals the CLR default) so + // IGlobalOptionsAccessor.GetValue resolves the same default as the injected T. var defaultValue = property.GetValue(prototype)?.ToString(); + var description = property.GetCustomAttribute()?.Description; - options.Parsing.AddGlobalOptionCore(name, property.PropertyType, aliases, defaultValue, typeof(T)); + options.Parsing.AddGlobalOptionCore(name, property.PropertyType, aliases, defaultValue, description, typeof(T)); } }); diff --git a/src/Repl.IntegrationTests/Given_CustomGlobalOptions.cs b/src/Repl.IntegrationTests/Given_CustomGlobalOptions.cs index f77b163..77e39ae 100644 --- a/src/Repl.IntegrationTests/Given_CustomGlobalOptions.cs +++ b/src/Repl.IntegrationTests/Given_CustomGlobalOptions.cs @@ -1,3 +1,5 @@ +using Repl.Parameters; + namespace Repl.IntegrationTests; [TestClass] @@ -45,5 +47,55 @@ public void When_RequestingRootHelp_Then_CustomGlobalOptionIsListedInGlobalOptio output.ExitCode.Should().Be(0); output.Text.Should().Contain("Global Options:"); output.Text.Should().Contain("--tenant, -t"); + output.Text.Should().Contain("Custom global option."); + } + + [TestMethod] + [Description("Regression guard: verifies typed global option descriptions are rendered in root help without adding default-value display.")] + public void When_RequestingRootHelpForTypedGlobalOptions_Then_DescriptionsAreListed() + { + var sut = ReplApp.Create() + .UseGlobalOptions(); + sut.Map("status", (DemoGlobals globals) => globals.Tenant); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["--help", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("--tenant, -t"); + output.Text.Should().Contain("Tenant id used for all commands."); + output.Text.Should().Contain("--verbose, -v"); + output.Text.Should().Contain("Enable verbose diagnostics for all commands."); + output.Text.Should().NotContain("[default:"); + } + + [TestMethod] + [Description("Regression guard: verifies explicit global option descriptions are rendered in root help.")] + public void When_RequestingRootHelpForExplicitGlobalOption_Then_DescriptionIsListed() + { + var sut = ReplApp.Create() + .Options(options => options.Parsing.AddGlobalOption( + "tenant", + aliases: ["-t"], + defaultValue: null, + description: "Tenant id used for all commands.")); + sut.Map("ping", () => "ok"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["--help", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("--tenant, -t"); + output.Text.Should().Contain("Tenant id used for all commands."); + output.Text.Should().NotContain("[default:"); + } + + private sealed class DemoGlobals + { + [System.ComponentModel.Description("Tenant id used for all commands.")] + [ReplOption(Aliases = ["-t"])] + public string? Tenant { get; set; } = "default"; + + [System.ComponentModel.Description("Enable verbose diagnostics for all commands.")] + [ReplOption(Aliases = ["-v"])] + public bool Verbose { get; set; } } } diff --git a/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs b/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs index 4cd8946..58f70f9 100644 --- a/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs +++ b/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs @@ -81,6 +81,53 @@ public void When_GlobalOptionNotProvided_Then_DefaultIsReturned() output.Text.Should().Contain("3000"); } + [TestMethod] + [Description("Regression guard: for typed global options, the accessor mirrors the prototype default even when it equals the CLR default (int = 0), staying consistent with the injected instance which always carries prototype values.")] + public void When_TypedGlobalOptionHasClrDefaultPrototypeValue_Then_AccessorMatchesInjectedInstance() + { + var sut = ReplApp.Create(); + sut.UseGlobalOptions(); + sut.Map("show", (IGlobalOptionsAccessor globals, ClrDefaultGlobals opts) => + $"accessor:{globals.GetValue("retries", 42)} injected:{opts.Retries}"); + + var output = ConsoleCaptureHelper.Capture( + () => sut.Run(["show", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("accessor:0 injected:0"); + } + + [TestMethod] + [Description("Regression guard: an implicit CLR default for a value type outside the primitive whitelist (Guid.Empty) is not stored as registration metadata, so the call-site fallback wins when the option is omitted.")] + public void When_GlobalOptionDeclaresImplicitGuidDefault_Then_CallSiteFallbackWins() + { + var fallback = new Guid(0x42424242, 0x4242, 0x4242, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42); + var sut = ReplApp.Create(); + sut.Options(o => o.Parsing.AddGlobalOption("session", aliases: null, defaultValue: default, description: "Session id.")); + sut.Map("show", (IGlobalOptionsAccessor globals) => $"session:{globals.GetValue("session", fallback)}"); + + var output = ConsoleCaptureHelper.Capture( + () => sut.Run(["show", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain($"session:{fallback}"); + } + + [TestMethod] + [Description("Regression guard: an explicit registration default equal to the CLR default of the underlying type (0), declared through a nullable type parameter, is preserved as metadata and applied when the option is omitted instead of the call-site fallback.")] + public void When_NullableGlobalOptionDeclaresUnderlyingClrDefault_Then_RegisteredDefaultWins() + { + var sut = ReplApp.Create(); + sut.Options(o => o.Parsing.AddGlobalOption("port", defaultValue: 0)); + sut.Map("show", (IGlobalOptionsAccessor globals) => $"port:{globals.GetValue("port", 8080)}"); + + var output = ConsoleCaptureHelper.Capture( + () => sut.Run(["show", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("port:0"); + } + [TestMethod] [Description("UseGlobalOptions registers typed class accessible via DI.")] public void When_UsingTypedGlobalOptions_Then_ClassIsPopulatedFromParsedValues() @@ -392,6 +439,11 @@ private sealed class TestGlobalOptions public int Port { get; set; } = 8080; } + private sealed class ClrDefaultGlobals + { + public int Retries { get; set; } + } + private interface IInterfaceGlobalOptions { string? Tenant { get; } diff --git a/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs b/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs index eb0fc75..a564ae4 100644 --- a/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs +++ b/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs @@ -32,6 +32,19 @@ public class PositionalSearchOptions public string Query { get; set; } = ""; } + [ReplOptionsGroup] + public class NullableDefaultsOptions + { + [ReplOption] + public int? Limit { get; set; } = 0; + + [ReplOption] + public bool? Force { get; set; } = false; + + [ReplOption] + public int Offset { get; set; } + } + [TestMethod] [Description("Regression guard: verifies named options bind to options group properties.")] public void When_UsingNamedOptionOnGroup_Then_PropertyBindsSuccessfully() @@ -125,6 +138,22 @@ public void When_UsingTwoGroups_Then_BothBindIndependently() output.Text.Should().Contain("json:20:5"); } + [TestMethod] + [Description("Regression guard: a nullable group property initialized to the CLR default of its underlying type (int? = 0, bool? = false) is a deliberate default the binder preserves, so command help must advertise it — while implicit defaults of non-nullable properties stay hidden.")] + public void When_NullableGroupPropertyInitializedToUnderlyingClrDefault_Then_HelpShowsDefault() + { + var sut = ReplApp.Create(); + sut.Map("list", (NullableDefaultsOptions options) => "ok"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["list", "--help", "--no-logo"])); + + output.ExitCode.Should().Be(0); + var lines = output.Text.Split('\n'); + lines.Single(line => line.Contains("--limit")).Should().Contain("[default: 0]"); + lines.Single(line => line.Contains("--force")).Should().Contain("[default: False]"); + lines.Single(line => line.Contains("--offset")).Should().NotContain("[default:"); + } + [TestMethod] [Description("Regression guard: verifies token collision between group and regular parameter fails at registration.")] public void When_GroupPropertyCollidesWithParam_Then_MapFails() diff --git a/src/Repl.Tests/Given_GlobalOptionsAccessor.cs b/src/Repl.Tests/Given_GlobalOptionsAccessor.cs index 2129a3f..34e066b 100644 --- a/src/Repl.Tests/Given_GlobalOptionsAccessor.cs +++ b/src/Repl.Tests/Given_GlobalOptionsAccessor.cs @@ -67,6 +67,49 @@ public void When_OptionNotProvidedButRegisteredDefault_Then_GetValueReturnsRegis sut.GetValue("port").Should().Be(3000); } + [TestMethod] + [Description("GetValue returns caller default when a value-typed option has no explicit registration default.")] + public void When_ValueTypeOptionNotProvidedAndNoRegisteredDefault_Then_GetValueReturnsCallerDefault() + { + var parsing = new ParsingOptions(); + parsing.AddGlobalOption("port"); + var sut = new GlobalOptionsSnapshot(parsing); + sut.Update(new Dictionary>(StringComparer.OrdinalIgnoreCase)); + + sut.GetValue("port", 8080).Should().Be(8080); + } + + [TestMethod] + [Description("GetValue returns caller default when a described value-typed option has no explicit registration default.")] + public void When_DescribedValueTypeOptionNotProvidedAndNoRegisteredDefault_Then_GetValueReturnsCallerDefault() + { + var parsing = new ParsingOptions(); + parsing.AddGlobalOption("port", aliases: null, defaultValue: default, description: "Port used by the server."); + var sut = new GlobalOptionsSnapshot(parsing); + sut.Update(new Dictionary>(StringComparer.OrdinalIgnoreCase)); + + sut.GetValue("port", 8080).Should().Be(8080); + } + + [TestMethod] + [Description("Regression (#34 review): a positional null second argument keeps binding the aliases-only overload. The description overloads take description as the trailing required parameter precisely so this call never becomes ambiguous (CS0121) with the description overload.")] + public void When_AddGlobalOptionCalledWithPositionalNull_Then_BindsAliasesOnlyOverload() + { + var parsing = new ParsingOptions(); + + // The positional null is the regression scenario itself: naming the argument would make + // resolution trivial and defeat the test. These calls must compile and bind unambiguously + // to the aliases-only overloads (description overloads take description as a trailing required arg). +#pragma warning disable MA0003 // Name the parameter — intentionally positional here; see comment above. + parsing.AddGlobalOption("tenant", null); + parsing.AddGlobalOption("port", "int", null); +#pragma warning restore MA0003 + + parsing.GlobalOptions["tenant"].Description.Should().BeNull(); + parsing.GlobalOptions["tenant"].Aliases.Should().BeEmpty(); + parsing.GlobalOptions["port"].Description.Should().BeNull(); + } + [TestMethod] [Description("HasValue returns false before parsing.")] public void When_NeverUpdated_Then_HasValueReturnsFalse()