refactor: remove type-check bypasses (cast/Any) with accurate types - #24
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThe PR introduces protocol-based service dependencies, typed test settings and source fakes, stricter LLM payload validation, and narrower annotations across geocoding, reference data, and weather-context parsing. Runtime processing behavior remains unchanged. ChangesTyped contracts and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@github-copilot please review this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #24 +/- ##
=======================================
Coverage 99.46% 99.46%
=======================================
Files 38 38
Lines 5224 5256 +32
Branches 303 303
=======================================
+ Hits 5196 5228 +32
Misses 17 17
Partials 11 11 ☔ View full report in Codecov by Harness. |
b0c7b4c to
4804e90
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the typing surface to remove cast() / cast(Any, ...) bypasses, replacing them with more precise type annotations and Protocol-based abstractions (notably for sources and service dependencies), while keeping JSON-boundary handling explicit.
Changes:
- Removed
cast()usage inweather_context.pyby introducing typed intermediate variables and adjusting helper signatures. - Introduced
RSSFeedSource/ContextDocumentSourceProtocols and updatedBriefingServiceto depend on Protocols (including a newBriefingSettingsProtocol). - Tightened
load_reference_data()return type and updated tests to removecast(Any, ...)by using typed test settings and aTypeGuardhelper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| weather_briefing/weather_context.py | Removes casts at JSON boundaries; adjusts QWeather formatting helper typing. |
| weather_briefing/sources.py | Adds Protocol abstractions for RSS/context sources. |
| weather_briefing/service.py | Switches BriefingService constructor dependencies to Protocols (settings + sources). |
| weather_briefing/reference_data.py | Narrows reference data loader return type to dict[str, object]. |
| tests/test_service.py | Removes test cast(Any, ...) by adding a typed settings dataclass and TypeGuard-based narrowing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace explicit cast() calls in weather_context.py with direct type annotations on variables assigned from response.json() (which returns Any). Change _format_qweather_lifestyle and _format_qweather_day parameter types from object to dict[str, object] so isinstance-narrowed dicts support string-key subscripting without cast. Tighten load_reference_data return type from dict[str, Any] to dict[str, object]—json.loads returns Any, isinstance narrows to dict[Any, Any], which is assignable to dict[str, object].
Add RSSFeedSource and ContextDocumentSource Protocols to sources.py, mirroring the existing LLMProvider and WeatherContextProvider pattern. BriefingService now depends on these Protocols plus a new BriefingSettings Protocol (read-only @Property members) instead of the concrete Settings, RSSSource, and HTTPContextSource classes. This removes all cast(Any, ...) calls in test_service.py: test doubles now satisfy the Protocols structurally, and a _TestSettings frozen dataclass replaces SimpleNamespace. Payload-access casts are replaced with a TypeGuard-based _is_dict_list helper.
4804e90 to
cfc6476
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
weather_briefing/geocoding.py (1)
137-137: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNarrow the nested JSON fields before using them as mappings/sequences.
The top-level
dict[str, object]annotations are fine, but these reads still treat nested API payloads as already narrowed. Add explicitdict/listchecks before using them to keep the boundary contract sound.
weather_briefing/geocoding.py#L137-L137: narrowresult["address"]before calling.get().weather_briefing/weather_context.py#L206-L206: narrowindices_payload["daily"]before iterating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@weather_briefing/geocoding.py` at line 137, Narrow nested API payloads before treating them as containers: in weather_briefing/geocoding.py around result, verify result["address"] is a dict before calling .get(); in weather_briefing/weather_context.py around indices_payload, verify indices_payload["daily"] is a list before iterating it. Preserve the existing behavior for valid payloads and handle invalid shapes safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@weather_briefing/geocoding.py`:
- Line 137: Narrow nested API payloads before treating them as containers: in
weather_briefing/geocoding.py around result, verify result["address"] is a dict
before calling .get(); in weather_briefing/weather_context.py around
indices_payload, verify indices_payload["daily"] is a list before iterating it.
Preserve the existing behavior for valid payloads and handle invalid shapes
safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ac3effe-c5f9-448c-aed8-b3da1b3ffb26
📒 Files selected for processing (6)
tests/test_service.pyweather_briefing/geocoding.pyweather_briefing/reference_data.pyweather_briefing/service.pyweather_briefing/sources.pyweather_briefing/weather_context.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
weather_briefing/weather_context.py:210
indices_payloadis annotated asdict[str, object], butindices_payload.get('daily', ())therefore has typeobject(or the default), which defeats the point of tightening the boundary type and can trigger type-checker errors (iterating anobject). Consider validating/narrowing thedailyfield to alistbefore iterating so both runtime and static typing are consistent.
indices_payload: dict[str, object] = {}
lifestyle_advice: tuple[str, ...] = ()
if forecast_date is None or str(forecast_date) == first_forecast_date:
operation = "lifestyle indices"
indices_response = await self._client.get(
The _format_qweather_lifestyle and _format_qweather_day helpers now accept dict[str, object] parameters. The runtime isinstance(item, dict) guards are redundant since callers already filter items at the JSON boundary (weather_context.py:198-199) or validate the response payload before iteration (weather_context.py:220-226). Non-dict items now raise TypeError (caught and wrapped by line 238) instead of ValueError.
- _is_dict_list TypeGuard now verifies string keys, making the narrowing to list[dict[str, object]] honest - Replace str(air_document["content"]) with explicit isinstance(content, str) assertion before substring checks
97cf2cb to
111039c
Compare
Summary
Remove all
cast()calls and tighten JSON-boundaryAnytypes across the codebase, replacing bypasses with accurate type annotations and Protocol-based abstractions.Changes
Production code
weather_context.py— removed 5cast()calls:response.json()results replaced with direct type-annotated assignments (daily: dict[str, list[object]] = payload["daily"],current: dict[str, object] = payload["current"])_format_qweather_lifestyle/_format_qweather_day: parameter type changed fromobjecttodict[str, object]soisinstance-narrowed dicts support string-key subscripting withoutcast_aqi_standardparameter tightened fromdict[str, Any]todict[str, object]indices_payloadtightened fromdict[str, Any]todict[str, object]geocoding.py— tightened 3 JSON-boundary annotations:_nominatim_result_matches/_open_meteo_result_matchesparameters:dict[str, Any]→dict[str, object]NominatimGeocodingProvider.geocoderesult variable:dict[str, Any]→dict[str, object]sources.py— addedRSSFeedSourceandContextDocumentSourceProtocols (mirrors existingLLMProvider/WeatherContextProviderpattern)service.py—BriefingServicenow depends on Protocols instead of concrete classes:BriefingSettingsProtocol (read-only@propertymembers) replaces concreteSettingsRSSFeedSource/ContextDocumentSourceProtocols replaceRSSSource/HTTPContextSourcereference_data.py— tightenedload_reference_datareturn type fromdict[str, Any]todict[str, object]Tests
test_service.py— removed all 80+cast(Any, ...)calls:_TestSettingsfrozen dataclass replacesSimpleNamespaceTypeGuard-based_is_dict_listhelper replaces payload-access castsVerification
ty check+ruff check+ruff format: cleanRemaining
AnyThe remaining
Anyusages are at JSON/API boundaries whereobjectis insufficient:float()-bound dicts (_parse_air_quality,_parse_allergen,_first_mapping, etc.) —float(object)rejected by type checker**-unpacked or iterated dicts (_read_cache,_json_file,parse_result) —objectnot unpackable/iterablehttpx.AsyncClient.sendkwargs)reference_value)All have runtime validation via
try/exceptorisinstancechecks.Summary by CodeRabbit
Refactor
Tests