refactor: simplify weather provider dispatch

[IceCodeNew]

Summary

• remove the weather builder registry that only dispatched Open-Meteo
• route every supported provider through its existing explicit builder
• eliminate the duplicated NEA construction path

Review context

Follow-up to the valid CodeRabbit Nitpick in #102 review 4763255472. The original PR was already merged before the finding was addressed.

Verification

• prek run --all-files
• .venv/bin/pytest --cov --cov-branch --cov-report=xml (910 passed, 99.87% line coverage)

[coderabbitai]

Warning

Review limit reached

@IceCodeNew, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 23e339c6-7053-4922-82f5-bb46b6ecbdcc

📥 Commits

Reviewing files that changed from the base of the PR and between 5c20301 and ca07e2a.

📒 Files selected for processing (2)

• tests/test_cli.py
• weather_briefing/composition/providers.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/simplify-weather-provider-dispatch

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[IceCodeNew]

/agentic_review

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.82%. Comparing base (5c20301) to head (ca07e2a).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #106   +/-   ##
=======================================
  Coverage   99.82%   99.82%           
=======================================
  Files          87       87           
  Lines        9535     9537    +2     
  Branches      563      563           
=======================================
+ Hits         9518     9520    +2     
  Misses         12       12           
  Partials        5        5           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 46 rules

✅ REVIEW.md

1. Lost dispatch exhaustiveness test ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The PR removes the unit test that enforced that every declared WeatherProviderName is covered by the
runtime dispatch, so future additions to WeatherProviderName can omit a build_weather_provider()
branch without being caught by an explicit completeness test. With the dispatch now implemented as
an explicit if-chain, this completeness invariant is otherwise unenforced and will only surface when
the new provider is selected (ValueError: unsupported).

Code

tests/test_cli.py[L1493-1494]

-def test_runtime_provider_builders_cover_declared_configuration_names() -> None:
-    assert set(WEATHER_PROVIDER_BUILDERS) == set(WeatherProviderName)
+def test_publisher_builders_cover_declared_configuration_names() -> None:

Relevance

⭐⭐⭐ High

They previously added this exact exhaustiveness test to guard provider/publisher registries; likely
want to keep it.

PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The repo still defines WeatherProviderName as the authoritative provider list, and
build_weather_provider now dispatches via an explicit if-chain that must be manually updated for
each provider; the PR simultaneously removes the prior explicit completeness assertion and replaces
it with a publisher-only coverage assertion.

weather_briefing/registries.py[6-13]
weather_briefing/composition/providers.py[252-269]
tests/test_cli.py[1492-1494]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A prior test ensured the runtime weather-provider dispatch stayed exhaustive with respect to the declared `WeatherProviderName` enum values. The PR deletes that check, but `build_weather_provider()` is now a manual `if`/`raise` chain; adding a new enum member without updating dispatch can now slip through CI unless another test happens to exercise it.

### Issue Context
- `WeatherProviderName` is the authoritative list of supported providers.
- `build_weather_provider()` now uses an explicit `if name == ...` chain and raises `ValueError` for anything else.
- The PR replaced the weather-provider coverage test with a publisher-builder coverage test.

### Fix
Reintroduce an explicit exhaustiveness test for weather providers, without resurrecting the removed registry.

One robust approach:
- Add a new parametrized test over `WeatherProviderName`.
- Call `_build_weather_provider(...)` for each name using `_make_fake_settings()`.
- Accept provider-specific `ValueError`s (e.g., missing QWeather config), but assert that the error is **not** the unsupported-provider error.
- For JMA, pass a dummy `jma_office_code` to avoid the unrelated "requires locations.json jma_office_code" failure.

Pseudo-shape:
```py
@pytest.mark.parametrize("name", WeatherProviderName)
async def test_build_weather_provider_dispatch_is_exhaustive(async_client, name):
   settings = _make_fake_settings()
   kwargs = {"jma_office_code": "130000"} if name == WeatherProviderName.JMA_JAPAN else {}
   try:
       provider = _build_weather_provider(name, settings, async_client, **kwargs)
   except ValueError as exc:
       assert "Unsupported weather provider" not in str(exc)
   else:
       assert provider is not None
```

### Fix Focus Areas
- tests/test_cli.py[1461-1495]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 30d23a0

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ca07e2a

[qodo-code-review]

PR Summary by Qodo

Refactor weather provider dispatch to explicit builders

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Remove the weather provider builder registry and dispatch via explicit builder functions.
• Route NEA through the shared builder to eliminate duplicate construction paths.
• Add tests that enforce dispatch exhaustiveness across all declared provider names.

Diagram

graph TD
  T["tests/test_cli.py"] --> B["build_weather_provider"]
  S["Settings"] --> B --> Q["_build_qweather"]
  C["httpx.AsyncClient"] --> B
  B --> O["_build_open_meteo"]
  B --> N["_build_nea"]
  B --> J["_build_jma"]

Loading

High-Level Assessment

The explicit dispatch keeps construction paths unified (notably for NEA) and avoids an extra registry layer that previously provided little value. With the new parameterized test asserting exhaustiveness over WeatherProviderName, this approach is both simpler and well-guarded against future enum additions.

Files changed (2) +22 / -16

Refactor (1) +4 / -13

providers.pyRemove WEATHER_PROVIDER_BUILDERS and make provider dispatch explicit +4/-13

Remove WEATHER_PROVIDER_BUILDERS and make provider dispatch explicit

• Eliminates the WEATHER_PROVIDER_BUILDERS registry and dispatches directly inside build_weather_provider for each supported provider. Routes NEA provider creation through _build_nea (removing a duplicate construction path) and raises a clear ValueError for unsupported providers.

weather_briefing/composition/providers.py

Tests (1) +18 / -3

test_cli.pyReplace registry coverage test with dispatch exhaustiveness test +18/-3

Replace registry coverage test with dispatch exhaustiveness test

• Removes the test that asserted WEATHER_PROVIDER_BUILDERS keys match WeatherProviderName. Adds a parameterized async test that builds each WeatherProviderName via build_weather_provider using minimally valid settings and asserts a provider instance is returned.

tests/test_cli.py

[qodo-code-review]

Code review by qodo was updated up to the latest commit ca07e2a