Skip to content

[claude] Defer change-type freeze until JsonSerializerOptions is first used#85

Closed
myieye wants to merge 1 commit into
mainfrom
claude/new-session-7248e6
Closed

[claude] Defer change-type freeze until JsonSerializerOptions is first used#85
myieye wants to merge 1 commit into
mainfrom
claude/new-session-7248e6

Conversation

@myieye

@myieye myieye commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Reading config.JsonSerializerOptions now freezes ChangeTypeListBuilder eagerly (introduced in #80), so a consumer that reads the options mid-configuration — e.g. to layer its own TypeInfoResolver modifier onto ours — and then registers more change types throws ChangeTypeListBuilder is frozen. This breaks FWL.

Fix: Make more stuff lazy.

PeekThenConcreteChangeConverter took the discriminator map eagerly, so
building the options froze ChangeTypeListBuilder. Consumers that read
config.JsonSerializerOptions mid-configuration (to add their own
TypeInfoResolver modifier) and then register more change types hit
"ChangeTypeListBuilder is frozen". Pass the map as a factory so the
converter resolves it on first use, restoring the pre-#80 freeze timing.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

CrdtConfig now defers discriminator map materialization, allowing additional change types to be registered after accessing serializer options. The converter resolves known types lazily, and a regression test verifies JSON round-tripping.

Changes

Change type registration

Layer / File(s) Summary
Lazy discriminator resolution
src/SIL.Harmony/Changes/PeekThenConcreteChangeConverter.cs
The converter accepts a discriminator-map factory and lazily resolves known types during discriminator matching and deserialization.
Configuration wiring and regression coverage
src/SIL.Harmony/CrdtConfig.cs, src/SIL.Harmony.Tests/ConfigTests.cs
CrdtConfig passes the lazy discriminator map, while a test registers a change type after accessing serializer options and verifies JSON round-tripping.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • sillsdev/harmony#80: Introduced the converter and opaque $type handling that this configuration-timing update extends.

Suggested reviewers: hahn-kev-bot, hahn-kev

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: deferring change-type freezing until JsonSerializerOptions is first used.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/new-session-7248e6

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@myieye
myieye requested a review from hahn-kev July 21, 2026 08:13
@myieye

myieye commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Actually, I'm thinking consumers/we can probably just avoid referencing the serialization options early.

@myieye myieye closed this Jul 21, 2026
@myieye
myieye removed the request for review from hahn-kev July 21, 2026 09:05
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.

1 participant