Skip to content

[repo-assist] fix: formatObject correctly handles Option(DateOnly) and Option(TimeOnly) properties#473

Merged
sergey-tihon merged 3 commits into
masterfrom
repo-assist/fix-formatobject-option-dateonly-20260718-4dce593f97db4d19
Jul 18, 2026
Merged

[repo-assist] fix: formatObject correctly handles Option(DateOnly) and Option(TimeOnly) properties#473
sergey-tihon merged 3 commits into
masterfrom
repo-assist/fix-formatobject-option-dateonly-20260718-4dce593f97db4d19

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

formatObject is used by the generated ToString() override on provider types to produce a debug-friendly {Prop=value; ...} string. Previously, Option<DateOnly> and Option<TimeOnly> properties fell through to obj.ToString(), which produces locale-specific output like Some(07/04/2025) instead of the ISO 8601 value that the rest of the serialization layer uses.

Root cause

The formatObject function handled DateOnly/TimeOnly scalars and arrays explicitly, but did not unwrap F# Option<T> before formatting. Option<DateOnly>.ToString() calls .NET's default box-then-print, giving Some(07/04/2025) in en-US locale or similar culture-dependent strings elsewhere.

Fix

  • Extracts a private appendFormattedValue helper (string → quoted, DateOnly/TimeOnly → ISO 8601, other types → ToString()) used by both the scalar and array branches, eliminating the duplicated formatting logic.
  • Adds an Option<T> unwrapping arm: Some(x) formats x via appendFormattedValue; None appends "null". Uses the existing optionTagReaderCache / optionValueCache infrastructure, so there is zero extra reflection cost after the first call per type.
  • In the array branch, uses the pre-computed elTy directly instead of calling x.GetType() on each element when isDateOnly/isTimeOnly is already known (minor performance improvement — addresses Task 8).

Trade-offs

  • No breaking changes: the output for existing types (non-option scalars, arrays, strings, ints) is unchanged. Only the previously-broken Option<DateOnly> and Option<TimeOnly> cases are affected.
  • The option-unwrapping arm also correctly formats Option<string> (produces "hello" instead of Some("hello")) and Option<int> (produces 42 instead of Some(42)), which are common in generated provider types.

Test Status

9 new tests added in FormatObjectTests covering:

  • Option<DateOnly> Some → ISO 8601, None → null
  • Option<TimeOnly> Some → HH:mm:ss.FFFFFFF, None → null
  • Option<string> Some → quoted, None → null
  • Option<int> Some → integer, None → null

All 540 tests pass (was 525 before this run; previous runs added other tests). Fantomas format check passes.

Generated by 🌈 Repo Assist, see workflow run. Learn more.

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@e15e57b40918dbca11b350c55d02ab61934afa75

…nly> properties

Previously, Option<DateOnly> and Option<TimeOnly> properties fell through to
obj.ToString(), producing locale-specific output like "Some(07/04/2025)"
instead of the ISO 8601 value used by toParam and the rest of the serialization
layer.

This commit:
- Extracts a private appendFormattedValue helper (string quoted, DateOnly/TimeOnly
  ISO 8601, other types via ToString) used by both the scalar and array branches.
- Adds an Option<T> unwrapping arm in formatObject: Some(x) formats x via
  appendFormattedValue; None appends "null".
- In the array branch, uses the pre-computed elTy directly instead of calling
  x.GetType() on each element when isDateOnly/isTimeOnly is already known.
- Adds 9 new tests covering Option<DateOnly>, Option<TimeOnly>, Option<string>,
  and Option<int> — both Some and None cases (540 tests total, was 525).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sergey-tihon
sergey-tihon marked this pull request as ready for review July 18, 2026 06:18
Copilot AI review requested due to automatic review settings July 18, 2026 06:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the runtime formatObject helper (used by generated provider types’ ToString() overrides) to format Option<DateOnly> / Option<TimeOnly> values using ISO-8601-style output instead of culture-dependent Option<T>.ToString() output.

Changes:

  • Extracts a private appendFormattedValue helper to centralize scalar formatting (string quoting, DateOnly/TimeOnly formatting).
  • Adds Option<T> unwrapping in formatObject so Some(x) formats x and None prints null (using cached reflection helpers).
  • Adds new unit tests intended to cover option formatting cases for DateOnly, TimeOnly, string, and int.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/SwaggerProvider.Runtime/RuntimeHelpers.fs Adds scalar formatting helper and an Option<T> formatting arm to formatObject.
tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Adds new FormatObjectTests cases for option values.
Comments suppressed due to low confidence (1)

tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs:1299

  • toFormUrlEncodedContent tests are now inside FormatObjectTests (the module ToFormUrlEncodedContentTests = header appears to have been removed). This changes the fully-qualified test names and makes filtering/running just the form-url-encoding tests harder, and is inconsistent with the rest of this file which groups tests by helper function in separate modules.


    [<Fact>]
    let ``toFormUrlEncodedContent encodes key-value pairs``() =
        task {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/SwaggerProvider.Runtime/RuntimeHelpers.fs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@sergey-tihon

Copy link
Copy Markdown
Member

@copilot fix the build

Copilot AI commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

@copilot fix the build

Addressed in 798612f. The PR build is now green on both Ubuntu and Windows, and I also re-ran the local build plus the SwaggerProvider.Tests unit test assembly successfully.

@sergey-tihon
sergey-tihon merged commit c68c477 into master Jul 18, 2026
5 of 6 checks passed
@sergey-tihon
sergey-tihon deleted the repo-assist/fix-formatobject-option-dateonly-20260718-4dce593f97db4d19 branch July 18, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants