refactor: make BaseInstrument generic; delete pure-annotation subclasses#95
Merged
Merged
Conversation
Closes DES-1: the four pure type-annotation framework subclasses (FastAPILoggingInstrument, FastAPISentryInstrument, LitestarSentryInstrument, FastStreamSentryInstrument) existed only to narrow bootstrap_config. They contributed no behavior. Make BaseInstrument generic in ConfigT (TypeVar bound to BaseConfig). Each base instrument concretely parameterizes the generic (LoggingInstrument(BaseInstrument[LoggingConfig]), etc.) and drops its own redundant bootstrap_config field declaration — the type is now provided by the parent's generic parameter. Delete the four pure-annotation framework subclasses. In the three affected bootstrappers' instruments_types lists, replace the deleted class references with the base names (SentryInstrument, LoggingInstrument). Framework subclasses that override bootstrap() with real framework-specific logic (FastAPICorsInstrument, LitestarLoggingInstrument, etc. — 15 in total) keep their existing structure including the bootstrap_config: <FrameworkConfig> annotation. These annotations are NOT redundant under the single-level-generic design adopted here — they provide essential type narrowing for framework field access (e.g., self.bootstrap_config.application inside FastAPICorsInstrument.bootstrap). This deviates from the sequencing spec's "drop redundant annotations on kept subclasses" decision: a two-level-generic design would be needed to make those annotations truly redundant, and the complexity isn't worth it. The audit's stated goal (eliminate the pure-annotation boilerplate) is fully addressed regardless. No behavior change. Existing tests verify correctness. Closes DES-1 from the audit.
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
1 participant
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
Closes DES-1 from an internal audit: the four pure type-annotation framework subclasses (
FastAPILoggingInstrument,FastAPISentryInstrument,LitestarSentryInstrument,FastStreamSentryInstrument) existed only to narrowbootstrap_config. They contributed no behavior.BaseInstrumentis now generic inConfigT(TypeVarbound toBaseConfig).LoggingInstrument(BaseInstrument[LoggingConfig]), etc.) and drops its redundantbootstrap_configfield declaration.instruments_typeslists now reference base names directly (SentryInstrument,LoggingInstrument).bootstrap()with framework-specific logic (15 total) keep their existing structure.12 files modified, net -24 lines. No behavior change. Existing 89 tests verify correctness.
Deviation from sequencing spec
The sequencing spec called for "dropping redundant
bootstrap_config:annotations on kept subclasses." Under single-level generic those annotations are NOT redundant — they provide essential type narrowing for framework field access (e.g.self.bootstrap_config.applicationinFastAPICorsInstrument.bootstrap()). Making them truly redundant would require a two-level-generic design (each base instrument with its own bounded TypeVar); the complexity isn't worth the marginal cleanup. The audit's stated goal — eliminating the pure-annotation boilerplate — is fully addressed regardless.Test plan
just test— 89/89.just lint— clean (ty checkclean,ruff checkclean).BaseInstrument'sslots=Trueworked first try with Generic (no fallback needed).# ty: ignorecomments required on framework subclasses'bootstrap_configre-annotations.# noqa: B027removal is acceptable. Ruff stopped flaggingabc.ABC + Generic[T]classes once Generic was added — the suppressions became genuinely unnecessary.bootstrap()andteardown()onBaseInstrumentare still concrete no-ops (overridable but not required); they're not abstract.Notes for reviewer
FastAPICorsInstrument,LitestarLoggingInstrument, etc.) still declarebootstrap_config: <FrameworkConfig>. This is a covariant narrowing (FrameworkConfigis a subtype ofLoggingConfig/CorsConfig/etc.) — standard dataclass pattern for narrowing inherited fields.SwaggerInstrumentended up with apassbody (only contained the dropped field declaration). Cosmetically odd but mechanically correct.🤖 Generated with Claude Code