Fix global option descriptions in help output#34
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Keep default values out of global option help
|
@codex Please do a full deep review here |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f9f94b3e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| internal static string? FormatDefaultValue(object? value, Type type) => | ||
| value is not null && !IsDefaultForType(value, type) | ||
| ? value.ToString() | ||
| : null; |
There was a problem hiding this comment.
When callers explicitly pass a registration default equal to the CLR default, for example AddGlobalOption<int>("port", defaultValue: 0) or AddGlobalOption<bool>("verbose", defaultValue: false), this helper returns null and discards that default. GlobalOptionsSnapshot.GetValue only applies registered defaults when definition.DefaultValue is non-null, so GetValue<int>("port", 8080) now returns the caller fallback instead of the registered 0; the previous generic overload preserved these explicit defaults. Consider separating “argument omitted” from “default(T) was supplied.”
Useful? React with 👍 / 👎.
| /// <param name="description">Optional description shown in help output.</param> | ||
| /// <param name="aliases">Optional aliases. Values without prefix are normalized to <c>--alias</c>.</param> | ||
| /// <param name="defaultValue">Optional default value metadata.</param> | ||
| public void AddGlobalOption<T>(string name, string? description, string[]? aliases = null, T? defaultValue = default) => |
There was a problem hiding this comment.
Adding this overload next to the existing AddGlobalOption<T>(string, string[]?, T?) makes existing consumer calls that pass a positional null for aliases, such as AddGlobalOption<string>("tenant", null), fail overload resolution because null can now mean either string? description or string[]? aliases. This is a public API, so apps using that no-alias form will stop compiling when upgrading; consider using a non-conflicting overload shape for descriptions.
Useful? React with 👍 / 👎.
The description-bearing AddGlobalOption overloads placed `description` as the
second parameter, colliding with the existing aliases-only overloads: a
positional null (e.g. `AddGlobalOption<string>("tenant", null)`) bound both
`string[]? aliases` and `string? description`, producing CS0121.
- Move `description` to a trailing required parameter on both the generic and
type-name overloads, so existing positional-null calls keep binding the
aliases-only overload. The original overloads are untouched (binary-safe).
- Update the two call sites that used the description overloads.
- Add a regression test asserting the positional-null call binds the
aliases-only overload.
ParsingOptions and HelpTextBuilder each carried a copy of IsDefaultForType; the help copy did not unwrap Nullable<T>, so the two could diverge for nullable value-typed defaults. Route the help renderer through ParsingOptions.IsDefaultForType and drop the duplicate.
2 participants
Executive Summary
Fixes #33 by carrying custom global option descriptions through registration metadata and rendering them in root help output.
Typed global options now use
[Description]metadata, and manually registered global options can provide an explicitdescription. Root help also appends default value metadata when available.Changes
Descriptionto global option metadata.descriptionparameters toParsingOptions.AddGlobalOption(...)overloads.UseGlobalOptions<T>()to readSystem.ComponentModel.DescriptionAttributefrom option properties.[default: ...], falling back toCustom global option.only when no description exists.Validation
dotnet test src/Repl.Tests/Repl.Tests.csprojdotnet test src/Repl.IntegrationTests/Repl.IntegrationTests.csproj