Repository navigation
cleanup: Remove dead build_full_topic and enforce realm-agnostic topic invariant - #273
Merged
Merged
Conversation
…c invariant
build_full_topic() composed {env}.{namespace}.{suffix} topics but had zero
callers in src/ since OMN-1972 established TopicResolver as the canonical
pass-through resolver. Leaving it exported invited accidental reintroduction
of env-prefixed topics.
- Delete util_topic_composition.py (build_full_topic, TopicCompositionError, MAX_NAMESPACE_LENGTH)
- Delete test_util_topic_composition.py (191 lines testing only dead code)
- Remove dead exports from topics/__init__.py
- Add IMPORTANT docstring in topics/__init__.py and topic_constants.py:
env prefixes must NOT appear on the wire; isolation via envelope identity
- Clarify topic_constants.py: DLQ is the sole exception to realm-agnostic rule
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR removes the topic composition utilities module and its tests, updates package exports to drop composition helpers, and updates docstrings/examples for DLQ topic naming and guidance to use TopicResolver for canonical topic handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…_topic_composition - Removed: topic_resolver.py:32 - See Also reference to deleted module Review iteration: 1/10
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
build_full_topic()(util_topic_composition.py) — zero callers insrc/since OMN-1972 establishedTopicResolveras the canonical pass-through resolver. The function composed{env}.{namespace}.{suffix}topics, an explicitly invalid pattern nowtest_util_topic_composition.py) that only exercised dead codebuild_full_topic,TopicCompositionError,MAX_NAMESPACE_LENGTH) fromtopics/__init__.pytopics/__init__.pyandtopic_constants.py: environment prefixes must NOT appear on the wire; isolation enforced via envelope identity and consumer group naming<env>.dlq.<category>.<version>) are the sole legitimate use of env prefixes — they're per-environment infrastructure storage, not event routingWhy
Leaving
build_full_topicexported was a footgun. It encoded a pattern that the architecture explicitly abandoned, and cargo-culting from its examples would reintroducedev.-prefixed topics that no consumer expects. Burning this bridge prevents drift.Test plan
poetry run pytest tests/unit/topics/ -xvs— 40 passed (TopicResolver + platform suffix tests)poetry run pytest tests/ -x -n auto— 3726 passed (5 pre-existing infra failures unrelated to this change)rg "build_full_topic" src/ tests/— zero hitsSummary by CodeRabbit
Documentation
Refactor