Added initial typehints#494
Conversation
# Conflicts: # rollbar/contrib/django/middleware.py # rollbar/test/test_rollbar.py
# Conflicts: # .github/workflows/ci.yml # pyproject.toml
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8102b7dca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
terencehonles
left a comment
terencehonles
left a comment
There was a problem hiding this comment.
This looks pretty extensive so that's nice to see. I see a couple things that you may want to consider.
You may want to consistently use from __future__ import annotations across the code. This does change the meaning of the imports at runtime and will require a consumer to resolve them differently, but since you didn't actually export the type comments I think this is a safe change to make and it makes sense to make it everywhere before you do export the type comments.
You're missing the py.typed marker, and you may want to add that. I'm not sure if you're waiting for more complete type coverage to export it.
You may want to lean into from __future__ import annotations by using modern syntax where not in a runtime position. I commented on a few of these and you can use a tool like pyupgrade to migrate these automatically, but it will change more than just the type definitions.
| return DEFAULT | ||
|
|
||
| # The keys will be coming from data objects in the payload, so they could be just about anything hashable. | ||
| KeyType = Union[str, int, float, binary_type, Hashable] |
There was a problem hiding this comment.
If you're using from __future__ import annotations does it make sense to just use the newer shorthand syntax:
| KeyType = Union[str, int, float, binary_type, Hashable] | |
| KeyType = str | int | float | binary_type | Hashable |
| allowed_circular_reference_types: Optional[type | tuple[type, ...]] = None, | ||
| memo: Optional[dict[int, tuple[KeyType, ...]]] = None, |
There was a problem hiding this comment.
If you're using from __future__ import annotations does it make sense to just use the newer shorthand syntax:
| allowed_circular_reference_types: Optional[type | tuple[type, ...]] = None, | |
| memo: Optional[dict[int, tuple[KeyType, ...]]] = None, | |
| allowed_circular_reference_types: type | tuple[type, ...] | None = None, | |
| memo: dict[int, tuple[KeyType, ...]] | None = None, |
|
|
||
| def transform(obj, transforms, key=None, batch_transforms=False): | ||
| class Handlers(TypedDict, total=False): | ||
| string_handler: Callable |
There was a problem hiding this comment.
These are all Callable[[Any, Any | None], Any] right? It might make sense to at least type this so that handlers have the right signature (via a typevar HandlerType would allow deduplication).
Also fixed failing FastAPI tests.
| ]) | ||
| app.add_middleware(ReporterMiddleware) | ||
|
|
There was a problem hiding this comment.
🔴 test_should_return_current_request (rollbar/test/starlette_tests/test_middleware.py:294-316) silently passes without running its assertion. The PR rewrites the test to put the assertEqual(get_public_attrs(request), get_public_attrs(original_request)) inside root(), but the route is registered as Route('/{param}', endpoint=root) while the client calls client.get('/'). Starlette's default path-parameter converter is [^/]+ (requires at least one non-slash character), so / does not match /{param} — the router returns 404, root() is never invoked, and the assertion never runs. Fix: change Route('/{param}', endpoint=root) to Route('/', endpoint=root) (or change the client call to client.get('/foo')).
Extended reasoning...
What the bug is
In rollbar/test/starlette_tests/test_middleware.py, the new test_should_return_current_request registers Route('/{param}', endpoint=root) and then issues client.get('/'). The assertion that exercises the contract under test —
async def root(original_request):
request = get_current_request()
self.assertEqual(get_public_attrs(request), get_public_attrs(original_request))
return PlainTextResponse('OK')— lives inside the root() handler. Starlette's default string convertor for path parameters is the regex [^/]+, which requires at least one non-slash character. Therefore the path / does not match /{param}, the router returns HTTP 404, root() is never invoked, and the assertEqual inside it never runs. The test passes vacuously while providing zero coverage of get_current_request() — exactly the contract the PR is overhauling for Starlette.
How this regressed
Before the PR, the test used the decorator form @app.route('/') which does match /. The refactor moved to the explicit Route() list style and apparently copy-pasted the '/{param}' pattern from the adjacent test_should_store_current_request. That neighboring test still works because its assertion is on the store_current_request mock (store_current_request.assert_called_once()), which the middleware invokes before routing happens — so it doesn't matter there that root() never executes. test_should_return_current_request has no such fallback: every meaningful assertion is inside the unreached handler.
Empirical reproduction
from starlette.applications import Starlette
from starlette.routing import Route
from starlette.testclient import TestClient
from starlette.responses import PlainTextResponse
called = False
async def root(request):
global called
called = True
return PlainTextResponse('OK')
app = Starlette(routes=[Route('/{param}', endpoint=root)])
client = TestClient(app)
r = client.get('/')
# r.status_code == 404, called == False, r.text == 'Not Found'
r2 = client.get('/foo')
# r2.status_code == 200, called == TrueThis was independently reproduced by every verifier that examined the bug.
Impact
Silent loss of test coverage for an exact behavior this PR is reworking — get_current_request() consistency in the Starlette ReporterMiddleware. A developer touching this code path could break the implementation and the test would happily continue to pass. CI cannot catch this kind of regression because the test reports success.
How to fix
One-character fix, either side works:
# Option A - fix the route to match
app = Starlette(routes=[Route('/', endpoint=root)])
# Option B - fix the client call to match the route
client.get('/foo')Option A is preferable since the test has no need for a path parameter.
Step-by-step proof
routes=[Route('/{param}', endpoint=root)]registers a single route whose path regex is^/(?P<param>[^/]+)$.client.get('/')issues a request with path'/'.- The path
'/'does not match^/(?P<param>[^/]+)$(the[^/]+requires at least one character), so the Starlette router falls through. - With no matching route, Starlette returns a 404 response and never calls the endpoint.
- Therefore
root()does not execute, and theself.assertEqual(...)on line 305 (insideroot()) is never evaluated. unittestrecords the test as passing because no assertion was raised — but no assertion was checked either.
| def report_exc_info( | ||
| exc_info: tuple[type[BaseException], BaseException, types.TracebackType] | tuple[None, None, None] | None = None, | ||
| request=None, extra_data=None, payload_data=None, level=None, **kw): |
There was a problem hiding this comment.
🟡 The new return annotation on report_exc_info declares the third tuple element as types.TracebackType (non-None) when the exception is non-None, but two internal callers in this file pass a potentially-None traceback in the non-None-exception branch: the Twisted log_handler at line 166 passes err.getTracebackObject() (Twisted's Failure.getTracebackObject() returns Optional[TracebackType]), and the lambda_function wrapper at line 624 passes trace.tb_next (TracebackType.tb_next is Optional[TracebackType]). The function body handles None tracebacks correctly (traceback.extract_tb(None) returns []), so runtime is fine and current mypy CI passes — both call sites live in untyped functions and aren't analysed in default mode. Filing as nit: widen to tuple[type[BaseException], BaseException, types.TracebackType | None] | tuple[None, None, None] | None so the annotation matches the actual contract.
Extended reasoning...
What the bug is
The new return annotation at rollbar/__init__.py:630-632:
def report_exc_info(
exc_info: tuple[type[BaseException], BaseException, types.TracebackType] | tuple[None, None, None] | None = None,
...):declares two alternatives for non-None tuples: either (type, exc, TracebackType) (all non-None) or (None, None, None). The third element must be a real TracebackType when the exception isn't None. But two internal callers in this same file pass tuples whose third element is Optional[TracebackType]:
-
rollbar/__init__.py:166(Twistedlog_handler) —report_exc_info((err.type, err.value, err.getTracebackObject())). Twisted'sFailure.getTracebackObject()is documented to returnOptional[TracebackType]; it returnsNonewhen the failure was constructed without a real traceback (which happens routinely with Failure's pickling/serialization paths). -
rollbar/__init__.py:624(lambda_functionwrapper) —report_exc_info((cls, exc, trace.tb_next)). Per the stdlib typeshed,TracebackType.tb_nextisTracebackType | None— it isNonewhen the exception was raised in the innermost frame, so the originating frame has no further frame to point to. (Also, from mypy's perspective inside anexcept:block,sys.exc_info()'s static return isUnion[tuple[type, exc, tb], tuple[None, None, None]], socls/exc/traceare allOptionalto start.)
Both produce tuple[type, exc, None] — which matches neither alternative of the declared union.
Why the function body is fine
The body delegates to _walk_trace_chain(cls, exc, trace) → _trace_data(cls, exc, trace) → traceback.extract_tb(trace). Per the Python docs, traceback.extract_tb(None) returns [], so the None case is handled cleanly without any branch needed. The actual contract the function accepts at runtime is therefore wider than the annotation declares.
Why mypy CI passes today (addressing the refutation)
A refuting verifier correctly pointed out that the project's mypy config ([tool.mypy] packages = ["rollbar"] in pyproject.toml) uses default settings without check_untyped_defs, so untyped function bodies are skipped. Both call sites live in untyped nested functions (log_handler and wrapper), so neither call site is currently analysed.
That's accurate, and is why this is filed as nit rather than normal severity — there is no runtime impact and the new types CI job demonstrably passes today. But the refutation does not establish that the annotation is correct; it only establishes that the inconsistency happens to be invisible to today's CI. Two reasons it's still worth fixing here:
-
The PR's stated goal is "the initial work of getting static type checking with mypy working" and it introduces the
typesjob. An annotation that contradicts the function's documented runtime contract is exactly the inconsistency that CI is meant to catch in follow-up work — the moment anyone annotateslog_handler/wrapper(a natural next step) or enablescheck_untyped_defs, real errors surface. -
External annotated callers using the idiomatic
report_exc_info((type(e), e, e.__traceback__))pattern would be flagged by mypy today, becauseBaseException.__traceback__isTracebackType | Noneper typeshed. So the bug isn't purely internal — it can affect any annotated user code that passes a triple.
Empirical reproduction
One verifier ran:
$ mypy rollbar/__init__.py --check-untyped-defs
rollbar/__init__.py:624: error: Argument 1 to 'report_exc_info' has incompatible type
'tuple[type[BaseException] | None, BaseException | None, TracebackType | Any | None]';
expected 'tuple[type[BaseException], BaseException, TracebackType] | tuple[None, None, None] | None' [arg-type]
And another verifier reproduced the same with an annotated caller using (type(e), e, e.__traceback__).
Step-by-step proof
- Read
rollbar/__init__.py:630-632— third element of the first union arm is the baretypes.TracebackType. - Read
rollbar/__init__.py:624—report_exc_info((cls, exc, trace.tb_next)). Per the stdlib typeshed,TracebackType.tb_next: TracebackType | None. - So a stack with a single frame (e.g.
def f(): raise RuntimeError) producestrace.tb_next is None, and the tuple passed is(RuntimeError, RuntimeError(...), None)— matching neither arm of the declared union. - Read
rollbar/__init__.py:166—report_exc_info((err.type, err.value, err.getTracebackObject())). Twisted typeshed:Failure.getTracebackObject() -> TracebackType | None. - Both call sites construct
tuple[type, exc, None]— which violates the declared signature. - Read
_trace_dataatrollbar/__init__.py— it callstraceback.extract_tb(trace). Per Python docs,traceback.extract_tb(None) == [], so the body handles it without an extra branch.
Suggested fix
One-line widening to match the actual contract:
def report_exc_info(
exc_info: tuple[type[BaseException], BaseException, types.TracebackType | None] | tuple[None, None, None] | None = None,
...):
| [dependency-groups] | ||
| test = [ | ||
| "blinker", | ||
| "httpx", | ||
| "pytest", | ||
| "python-multipart", | ||
| "webob", |
There was a problem hiding this comment.
🟡 Test file rollbar/test/fastapi_tests/test_routing.py:6 directly imports from packaging.version import Version (new in this PR for the version comparison fix), but packaging is not declared in the test dependency group in pyproject.toml. This currently works only transitively because pytest declares packaging>=22 as a runtime requirement; declaring direct imports as direct deps is the same hygiene the PR already applied for typing_extensions (added to runtime deps because the PR newly imports it). One-line fix: add "packaging", to the test group around line 48.
Extended reasoning...
What the bug is
This PR's rollbar/test/fastapi_tests/test_routing.py newly imports from packaging.version import Version at line 6 — the import replaces an earlier lexicographic fastapi.__version__ >= '0.41.0' comparison with a proper Version(...) comparison (line 13), and is also used inside test_should_send_payload_with_request_body to gate on Version(httpx_version) >= Version('0.28.0'). The test dependency group in pyproject.toml (lines 47–54), however, lists only:
test = ["blinker", "httpx", "pytest", "python-multipart", "webob"]No packaging entry.
Why it works today (and the refutation)
A refuting verifier correctly pointed out that pytest declares packaging>=22 as a hard runtime dependency, so any environment that installs the test group transitively pulls packaging. CI therefore never sees an ImportError. That argument is accurate, and is exactly why this is filed as a nit rather than normal severity — there is no current breakage and the practical risk is near-zero because pytest has required packaging for many releases.
Why it's still worth flagging
The same PR explicitly applied the opposite convention one section above: it added typing_extensions; python_version < "3.11" to the runtime dependencies list (pyproject.toml:44) precisely because the PR newly introduced from typing_extensions import Unpack in rollbar/__init__.py. The 'if you directly import it, declare it' rule is therefore the rule this PR itself sets. Leaving packaging as a transitive-only dep contradicts that rule by one line in the same file.
Relying on transitive dependencies for direct imports is a well-known anti-pattern (PEP 508 / packaging best practices). The failure mode is small but real: if pytest ever drops its packaging dep (theoretical, but not impossible across major-version bumps), or if a downstream user installs the test target with a minimal resolver, the import fails at collection time because the import is at module scope and pytest evaluates it before any skipUnless guard runs.
How to fix
Single-line change in pyproject.toml around line 48:
test = [
"blinker",
"httpx",
"packaging",
"pytest",
"python-multipart",
"webob",
]Step-by-step proof
- Read
rollbar/test/fastapi_tests/test_routing.py:6—from packaging.version import Version. Unconditional, module-scope import (pytest collection alone triggers it). - Read
rollbar/test/fastapi_tests/test_routing.py:13—ALLOWED_FASTAPI_VERSION = Version(fastapi.__version__) >= Version('0.41.0'). Confirms the import is actually used. - Read
pyproject.toml:47-54—testgroup hasblinker, httpx, pytest, python-multipart, webob. Nopackaging. - Read
pyproject.toml:42-45— runtimedependenciesnow includestyping_extensions; python_version < "3.11". Confirms this PR's own convention: 'direct import → direct declaration'. packagingis currently available in every test environment only becausepytest(in thetestgroup) declaresRequires-Dist: packaging>=22in its METADATA. That is a transitive guarantee, not a direct one.
| for handler_type, handler in custom_handlers.items(): | ||
| if isinstance(obj, handler_type): | ||
| # Only attempt isinstance checks when the key is a type (or tuple of types). | ||
| if isinstance(handler_type, (type, tuple)) and isinstance(obj, handler_type): | ||
| return handler(obj, key=key) |
There was a problem hiding this comment.
🟡 Minor: the new defensive gate at traverse.py:176-179 (isinstance(handler_type, (type, tuple)) and isinstance(obj, handler_type)) is unreachable. **custom_handlers collects kwargs into a dict whose keys Python guarantees to be str (passing a non-string key raises TypeError: keywords must be strings at call time), so isinstance(handler_type, (type, tuple)) is provably always False. No caller in rollbar/ or tests populates this slot today, so there is zero runtime impact — but the gate quietly hides what would previously have surfaced as a TypeError and encodes a contract ("type-keyed handlers") that **kwargs makes impossible to satisfy. Suggest either dropping **custom_handlers from the signature or converting it to an explicit custom_handlers: dict[type, Handler] | None = None parameter so type-keyed entries are actually expressible.
Extended reasoning...
What the bug is
traverse() declares **custom_handlers: Handler (rollbar/lib/traverse.py:94), then near the end of the body (lines 175–178) iterates that dict and applies an isinstance gate:
elif obj_type is DEFAULT:
for handler_type, handler in custom_handlers.items():
# Only attempt isinstance checks when the key is a type (or tuple of types).
if isinstance(handler_type, (type, tuple)) and isinstance(obj, handler_type):
return handler(obj, key=key)The new isinstance(handler_type, (type, tuple)) clause was added in this PR (the prior code had only isinstance(obj, handler_type), which would have raised TypeError if handler_type was ever a non-type — but as we will see, it never can be).
Why the inner branch is unreachable
Python enforces at call time that keys absorbed by a **kwargs parameter are str. Empirically reproducing:
>>> def f(**kw): return list(kw.keys())
>>> f(**{int: 'x'})
TypeError: keywords must be stringsSo inside traverse(), custom_handlers is statically dict[str, Handler]. handler_type is always a str. isinstance('some_string', (type, tuple)) is unconditionally False, so the return handler(obj, key=key) body is never reached.
The propagation via kw.update(custom_handlers) at line 123 doesn't help either — it preserves the original string keys throughout the recursion.
Why no caller is actually affected today
$ grep -rn 'custom_handlers' rollbar/
rollbar/lib/traverse.py:94: **custom_handlers: Handler,
rollbar/lib/traverse.py:123: kw.update(custom_handlers)
rollbar/lib/traverse.py:176: for handler_type, handler in custom_handlers.items():
The only matches are the definition, the recursive propagation, and the dead loop itself. Every internal caller passes only the named handler arguments (string_handler, tuple_handler, namedtuple_handler, …, circular_reference_handler), which are absorbed by the explicit parameters and never reach custom_handlers. The production call site at rollbar/lib/transforms/__init__.py:110 passes those same named kwargs through. So custom_handlers is always an empty dict at runtime, and the loop is never entered to begin with.
What the PR change actually accomplishes
The new isinstance(handler_type, (type, tuple)) clause is most plausibly a mypy-silencer: isinstance(obj, handler_type) where handler_type: str would be a static-type error under the new types CI job. Adding the guard makes mypy happy without removing the dead loop. The net effect is that the PR papers over an inconsistency rather than fixing it — a reader sees an apparent extensibility hook (**custom_handlers for type-keyed handlers) that, by virtue of **kwargs semantics, cannot actually be used as designed.
Impact
Zero runtime impact. The feature has no callers, and even if someone tried to populate it via traverse(obj, my_extra=some_handler), the key would be the string 'my_extra', not a type, so the gate would skip it and default_handler would run as it does today. Severity: nit — a cleanup concern in code the PR is touching, not a behavior change.
Step-by-step proof
def traverse(..., **custom_handlers: Handler)(traverse.py:80–94) —custom_handlersis collected from caller kwargs.- At call time, Python rejects any non-string kwarg key:
f(**{int: 'x'})raisesTypeError: keywords must be strings. So at runtimecustom_handlersisdict[str, Handler]. - Therefore at traverse.py:176,
handler_typeis always astr. isinstance('any string', (type, tuple))isFalsefor every string (no string is a subclass oftypeortuple). The conjunction is unconditionallyFalse.- The
return handler(obj, key=key)body at line 178 is therefore unreachable. grep -rn custom_handlers rollbar/returns three hits, all internal totraverse.py; no caller populates the slot anyway, so even the outerforloop is never entered with a non-empty dict.
How to fix
Either of:
# Option A: drop the parameter entirely.
def traverse(obj, key=(), string_handler=..., ..., depth_first=True):
...# Option B: convert to an explicit, type-keyed parameter.
def traverse(obj, ..., custom_handlers: dict[type, Handler] | None = None):
...
elif obj_type is DEFAULT:
for handler_type, handler in (custom_handlers or {}).items():
if isinstance(obj, handler_type):
return handler(obj, key=key)Option B preserves the apparent design intent (extensibility for custom types) and makes it actually reachable. Option A is the minimal cleanup if the feature is genuinely unwanted.
3 participants
Description of the change
This PR adds the initial work of getting static type checking with mypy working. Because we support Python 3.9 to 3.14 and have integrations with a lot of frameworks that may or may not exist in the environment, our type imports can be a little messy at times. As much as possible we try not to ignore types.
Type of change
Related issues
Checklists
Development
Code review