Skip to content

fix: resolve nullable type lookup bypassing custom value parsers#608

Merged
natemcmaster merged 2 commits intomainfrom
claude/issue-559-20260405-0553
Apr 5, 2026
Merged

fix: resolve nullable type lookup bypassing custom value parsers#608
natemcmaster merged 2 commits intomainfrom
claude/issue-559-20260405-0553

Conversation

@natemcmaster
Copy link
Copy Markdown
Owner

Fix nullable type lookup in ValueParserProvider.GetParserImpl<T>() to check user-registered parsers under the unwrapped type before falling back to TypeDescriptor.

This fixes the bug where AddOrReplace with a custom parser for a built-in type like TimeSpan was ignored when the property type was TimeSpan?.

Fixes #559

Generated with Claude Code

Move the nullable type unwrap check before the TypeDescriptor fallback
in GetParserImpl<T>(). Previously, when a custom parser was registered
via AddOrReplace for a type like TimeSpan, looking up TimeSpan? would
miss it because the TypeDescriptor fallback (which returns the built-in
converter) ran before the nullable unwrap that checks the dictionary
under the unwrapped type.

Fixes #559

Co-authored-by: Nate McMaster <natemcmaster@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude bot commented Apr 5, 2026

Claude finished @natemcmaster's task in 1m 7s —— View job


Code Review: PR #608

  • Read review instructions
  • Analyze PR diff
  • Review changed files in detail
  • Post review feedback

Thanks for the fix — this is clean and correct.

The bug: In GetParserImpl<T>(), the _defaultValueParserFactory.TryGetParser<T>() fallback (which uses TypeDescriptor) was checked before the nullable-unwrap block that looks up _parsers[wrappedType]. So for TimeSpan?, the default factory would return a built-in parser for the nullable type, and the user-registered parser for TimeSpan was never consulted.

The fix: Moving the default factory fallback after the nullable unwrap ensures the lookup order is:

  1. _parsers[TimeSpan?] (exact match)
  2. Unwrap nullable → _parsers[TimeSpan] (user-registered for inner type) → wrap in NullableValueParser
  3. _defaultValueParserFactory (built-in fallback)

This correctly prioritizes user-registered parsers over the built-in defaults for nullable types. For non-nullable types, the nullable block is skipped entirely, so behavior is unchanged.

Test coverage is solid — the three new tests cover custom parser with nullable, custom parser with non-nullable, and default behavior without a custom parser.

No issues found. ✅

@natemcmaster
Copy link
Copy Markdown
Owner Author

@claude update this PR with release notes on the fix

@claude
Copy link
Copy Markdown

claude bot commented Apr 5, 2026

Claude finished @natemcmaster's task in 40s —— View job


Updated PR with release notes

  • Read current release notes files
  • Update releasenotes.props and CHANGELOG.md
  • Commit and push changes

Added release notes for the nullable value parser fix (#559) to both:

  • CHANGELOG.md — under the Unreleased Fixes section
  • src/CommandLineUtils/releasenotes.props — under the 5.1.x Fixes section

Co-authored-by: Nate McMaster <natemcmaster@users.noreply.github.com>
@natemcmaster natemcmaster merged commit 00c331e into main Apr 5, 2026
1 check passed
@natemcmaster natemcmaster deleted the claude/issue-559-20260405-0553 branch April 5, 2026 06:02
This was referenced Apr 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

How to override default parser for a specific TimeSpan option?

1 participant