Add regex-based module resolver rules#5
Merged
Conversation
The previous set_module_resolver API was a global singleton callable, which made it impossible for libraries that ship with proto_converter converters to each install their own resolvers — any library-level call would stomp on the application's resolver. Replace with add_module_resolver_rule(pattern, replacement), which: - Takes a regex pattern (using the `regex` package for safer matching) and a str.format-style replacement template - Supports named capture groups (keyword args) and positional groups - Composes via a list of rules — multiple calls register independent rules that coexist, so libraries can each add their own without coordinating - Detects configuration bugs: duplicate patterns with different replacements raise ValueError; multiple rules matching the same module path raise ValueError at converter construction time (via _descriptor_to_type, which runs during ProtoConverter.__init__) Startup ordering is unchanged: rules still need to be registered before any converter that depends on them is constructed. The composability fix only addresses the multiple-libraries case. set_module_resolver is kept for backward compatibility but emits a DeprecationWarning. Bump to 1.1.0 (minor version, backward-compatible additions + deprecation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Make `remove_module_resolver_rule` idempotent (no longer raises on missing)
- Detect "near-miss" rules (pattern matches prefix but not full) and surface them
in the error with a hint to add `(.+)`. Raise `ValueError` so it propagates
through the parent's `except (NotImplementedError, RuntimeError)` handler
- Document 0-indexed capture groups in replacements (str.format convention)
and brace escaping (`{{` / `}}`)
- Clarify that ambiguous matches raise at converter construction, not conversion
- Drop `set_type_resolver` references from deprecation warning + README
- Add test protos (`ext_api.remote.v1` / `ext_internal.remote.v1`) whose proto
packages intentionally don't match their Python module paths, so resolver
rule tests exercise real descriptor resolution
There was a problem hiding this comment.
Pull request overview
This PR updates proto-converter’s module-resolution mechanism to support composable, regex-based remapping rules, while keeping the old singleton resolver API as a deprecated fallback.
Changes:
- Added
add_module_resolver_rule()/remove_module_resolver_rule()and integrated rule application into descriptor→type resolution. - Deprecated
set_module_resolver()(now emitsDeprecationWarning) and updated docs/tests accordingly. - Added new remapped test protos + generation/build wiring; bumped version to
1.1.0and addedregexas a dependency.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/proto_converter/converter.py |
Implements rule registry + application, and deprecates the old module resolver hook. |
src/proto_converter/__init__.py |
Exposes the new public API functions. |
tests/test_converter.py |
Adds coverage for rule behavior, ambiguity detection, near-miss errors, and deprecation warnings. |
tests/protos/remapped_api/api.proto |
New test proto with proto package intentionally differing from Python module path. |
tests/protos/remapped_internal/internal.proto |
New internal-side counterpart for remapping tests. |
tests/gen_protos.sh |
Generates the new remapped protos and makes them importable. |
tests/test_protos/pyproject.toml |
Ensures remapped generated packages are included in the test-protos wheel. |
README.md |
Documents the new regex-based rule API and its semantics. |
pyproject.toml |
Bumps project version and adds regex dependency. |
uv.lock |
Locks new dependency and reflects version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Source-level `\\` in the example was confusing readers into thinking the pattern matched a literal backslash. The rendered docstring was already correct (normal triple-quoted strings turn `\\` into `\`), but a raw docstring makes source and rendered output match.
petersalas
approved these changes
Apr 20, 2026
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
Replaces the global
set_module_resolver(callable)singleton with an additiveadd_module_resolver_rule(pattern, replacement)API that supports library composition.Why: The previous singleton meant a library that ships with
proto_converterconverters couldn't install its own module resolver without stomping on the application's resolver (or another library's). With rule-based registration, libraries each register their own rules independently.API:
add_module_resolver_rule(pattern, replacement)— regex pattern,str.format-style replacement with named and positional capture groupsremove_module_resolver_rule(pattern)— inverseset_module_resolver(callable)— kept for back-compat, now emitsDeprecationWarningSafety guarantees:
regexpackage (not stdlibre) to prevent catastrophic backtracking on user patternsValueError(real bug)ValueErrorat construction time (surfaces immediately)What this doesn't fix: startup ordering. Rules still need to be registered before any converter that depends on them is constructed. Composability is the specific problem addressed here.
Version bumped to 1.1.0 (backward-compatible additions + deprecation).
Test plan
just— 48 tests pass, ruff/pyright/deptry clean🤖 Generated with Claude Code