Allow replacing unused auto-created converters#4
Closed
mdepinet wants to merge 2 commits into
Closed
Conversation
When get_converter() auto-creates a converter for a type pair, it now uses _AutoConverter (an internal subclass) that tracks whether convert() has been called. A ProtoConverter subclass registered later can replace an unused _AutoConverter, eliminating class-definition ordering constraints in most cases. Once an _AutoConverter has been used (convert() called), it's locked in and replacement raises RuntimeError — preserving the invariant that convert() behavior doesn't change after first use. Ordering still matters when a nested type can't be auto-converted (e.g. field names differ) — the custom subclass must be defined before the parent tries to auto-resolve it. Bump to 1.0.2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
_AutoConverter.convert() now checks the registry and delegates to the replacement if it's been replaced. This ensures parent closures that captured the _AutoConverter instance use the latest registered converter, not the stale auto-created one. Added test verifying that nested conversions through a parent use a replacement subclass, not the original auto-converter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing per review feedback — the strict ordering rule is simpler, more predictable under schema evolution, and already necessary for non-auto-convertible subtypes. The partial relaxation adds complexity without meaningful benefit since real codebases adopt leaf-first ordering anyway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When
get_converter()auto-creates a converter for a type pair, it now uses_AutoConverter(an internal subclass) that tracks whetherconvert()has been called. AProtoConvertersubclass registered later can replace an unused_AutoConverter, eliminating class-definition ordering constraints in most cases.This was the main friction point during the Fixie migration — parent converters had to be defined after all their leaf converters so that auto-resolution would find the registered subclasses. With this change, definition order is irrelevant for auto-convertible subtypes as long as all subclasses are defined before the first
convert()call. Subtypes that cannot be auto-converted (e.g. field names differ between source and destination) still need their custom converter defined before the parent.Invariant preserved: once a converter has been used (
convert()called), it's locked in and replacement raisesRuntimeError. This ensuresconvert()behavior doesn't change after first use.Unhandled field errors are still caught at construction time. If a nested type genuinely can't be auto-converted, auto-creation fails with
NotImplementedErrorwhen the parent registers — exactly as before. The only thing that changes is: if auto-creation succeeds, the result can be replaced by a subclass later.Test plan
just— 41 passed, pyright/ruff/deptry cleantest_replace_unused_auto_converter— verifies replacement succeedstest_replace_used_auto_converter_fails— verifies used converters are locked🤖 Generated with Claude Code