feat: add postcode lookup for malvern hills district council#2074
feat: add postcode lookup for malvern hills district council#2074InertiaUK wants to merge 2 commits into
Conversation
The scraper now resolves UPRNs from postcode + house number using the council's own address lookup API, removing the need for users to find and supply a UPRN manually. UPRN-only input still works. Also adds a null check on the results table to give a clear error when the address is not found in bin round records.
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 20 minutes and 55 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMalvernHillsDC now supports resolving UPRN via postcode and house number through a council address lookup API. The scraper accepts postcode/house_number to derive UPRN or accepts UPRN directly. A new ChangesMalvern Hills UPRN Lookup Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
uk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py (2)
88-90: ⚡ Quick winAlign disambiguation error wording with the actual CLI argument.
The user-facing argument is
paon(seekwargs.get("paon")on line 20 and theinput.jsonchange in this PR), but the error tells them to providehouse_number, which they have no way to pass. Mentionpaon(or both) so the message is actionable.✏️ Proposed wording tweak
- raise ValueError( - f"Multiple addresses found for {postcode} — provide house_number to disambiguate" - ) + raise ValueError( + f"Multiple addresses found for {postcode} — provide -p/--paon (house number) to disambiguate" + )🤖 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 `@uk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py` around lines 88 - 90, The disambiguation error message in MalvernHillsDC.py currently tells users to provide "house_number" which doesn't match the actual CLI/input argument "paon" (see kwargs.get("paon")); update the ValueError message raised where multiple addresses are found to reference "paon" (or both "paon" and "house_number") so the instruction is actionable for users, e.g., mention "provide paon (house number)" or "provide paon/house_number" in the exception raised.
63-72: ⚡ Quick winTighten the signature and guard the outbound HTTP call.
Two small follow-ups on
_resolve_uprn:
- Line 63 trips Ruff
RUF013(PEP 484 forbids implicitOptional). Annotatehouse_numberasOptional[str](orstr | None) and default toNone.- Line 71 has no
timeout=onrequests.get, so a hung lookup server can block the scraper indefinitely. Adding a sensible timeout will fail fast and surface as arequestsexception that the caller already understands.♻️ Proposed diff
-from bs4 import BeautifulSoup +from typing import Optional + +from bs4 import BeautifulSoup @@ - def _resolve_uprn(self, postcode: str, house_number: str = None) -> str: + def _resolve_uprn(self, postcode: str, house_number: Optional[str] = None) -> str: params = { "simple": "T", "pcode": postcode, "authority": "MHDC", "historical": "false", "hidedummyuprn": "1", } - response = requests.get(self.UPRN_LOOKUP_URL, params=params, verify=False) + response = requests.get( + self.UPRN_LOOKUP_URL, params=params, verify=False, timeout=30 + ) response.raise_for_status()🤖 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 `@uk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py` around lines 63 - 72, The _resolve_uprn function signature and HTTP call need tightening: annotate house_number as Optional[str] (or use str | None) in the _resolve_uprn signature to satisfy Ruff RUF013 and update any needed typing imports, and add a sensible timeout parameter to the requests.get call (e.g., timeout=10) when calling self.UPRN_LOOKUP_URL so the outbound lookup fails fast and raises a requests exception instead of hanging.
🤖 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.
Inline comments:
In `@uk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py`:
- Around line 78-83: The current logic in MalvernHillsDC.py uses
addr.startswith(house_lower) which can incorrectly match prefixes (e.g., "1"
matching "148 …"); change the comparison to check the first whitespace-delimited
token of entry["Address_Short"] against house_lower (or use a word-boundary
regex like rf"^{re.escape(house_lower)}\b") so only an exact house-number token
(not a prefix) returns entry["UPRN"]; keep using the same variables
(house_number/house_lower, results, entry.get("Address_Short"), entry["UPRN"])
and ensure you lower/strip the token before comparing.
---
Nitpick comments:
In `@uk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py`:
- Around line 88-90: The disambiguation error message in MalvernHillsDC.py
currently tells users to provide "house_number" which doesn't match the actual
CLI/input argument "paon" (see kwargs.get("paon")); update the ValueError
message raised where multiple addresses are found to reference "paon" (or both
"paon" and "house_number") so the instruction is actionable for users, e.g.,
mention "provide paon (house number)" or "provide paon/house_number" in the
exception raised.
- Around line 63-72: The _resolve_uprn function signature and HTTP call need
tightening: annotate house_number as Optional[str] (or use str | None) in the
_resolve_uprn signature to satisfy Ruff RUF013 and update any needed typing
imports, and add a sensible timeout parameter to the requests.get call (e.g.,
timeout=10) when calling self.UPRN_LOOKUP_URL so the outbound lookup fails fast
and raises a requests exception instead of hanging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6854851-1abe-4c42-aa01-9f626109621c
📒 Files selected for processing (2)
uk_bin_collection/tests/input.jsonuk_bin_collection/uk_bin_collection/councils/MalvernHillsDC.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2074 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
1 participant
Summary
sw2AddressLookupWS) for postcode-to-UPRN resolutionThe existing scraper wasn't broken — it works fine with a valid UPRN. This change removes the need for users to find and supply a UPRN manually, which is a common pain point (see #1497).
Testing
WR14 1AA+ UPRN100120606212WR14 1AA+ paon148Summary by CodeRabbit
New Features
Bug Fixes