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".
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a40cc974a
ℹ️ 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".
IsDefaultForType unwrapped Nullable<T> before comparing against the CLR
default, so a nullable options-group property initialized to 0 or false
(e.g. `int? Limit { get; set; } = 0`) had its default hidden from command
help, and AddGlobalOption<T?> discarded explicit CLR-default metadata that
GlobalOptionsSnapshot.GetValue would otherwise apply.
- Return false from IsDefaultForType for Nullable<T>: its default is null,
so any non-null value is a deliberate default.
- Add regression tests for group-property help rendering and for runtime
resolution of an explicit nullable global default.
Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8f3d88e6e
ℹ️ 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".
IsDefaultForType only recognized bool/int/long/double, so the implicit default captured by `T? defaultValue = default` for other value types (Guid, decimal, DateTime, enums) was stored as registration metadata and GlobalOptionsSnapshot.GetValue returned it instead of the call-site fallback. - Compare any non-nullable value type against its boxed CLR default instead of a primitive whitelist (registration-time only). - Add a regression test for the implicit Guid.Empty case. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac036ffa98
ℹ️ 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".
UseGlobalOptions<T> filtered CLR-default prototype values out of the registration metadata, so IGlobalOptionsAccessor.GetValue returned the call-site fallback while the injected T instance kept the prototype value (PopulateInstance starts from new T() and only overwrites parsed values). - Store the prototype value verbatim for typed global options so both access paths resolve the same default. - Add a regression test comparing accessor and injected-instance values for a CLR-default prototype property. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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