feat: SheffieldCityCouncil - add postcode + house number lookup#2060
feat: SheffieldCityCouncil - add postcode + house number lookup#2060InertiaUK wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 23 minutes and 12 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)
📝 WalkthroughWalkthroughMigrates Sheffield council bin collection from HTML scraping to Sheffield Waste Services API: searches properties by postcode, matches by UPRN/PAON for pointId, fetches collection days, filters out completed services, formats scheduled dates, sorts them, and returns {"bins": [...]}. ChangesSheffield API Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 2
🤖 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/SheffieldCityCouncil.py`:
- Around line 94-95: In the schedule date parsing block inside the
SheffieldCityCouncil scraper (class SheffieldCityCouncil), stop silently
swallowing ValueError/TypeError; instead capture the original exception and
re-raise (or raise a new descriptive exception) including the offending date
string and context so upstream callers see the failure; if you have a logger
(e.g. self.logger or module logger) emit an error with the raw value before
raising to aid debugging.
- Around line 24-27: The loop over properties is doing fuzzy substring matching
via "if paon_norm in name" which causes wrong matches (e.g., "2" matching "22");
change this to exact PAON equality by replacing that condition with an exact
comparison (e.g., compare the normalized name to paon_norm using ==) so the code
returns prop["id"] only on an exact PAON match; keep the existing normalization
(str(...).upper()) and leave the later fallback behavior intact.
🪄 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: 3b86f3f0-0ee4-4bad-a416-7e0c709f3a10
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/SheffieldCityCouncil.py
| except (ValueError, TypeError): | ||
| continue |
There was a problem hiding this comment.
Don’t silently swallow schedule date parsing failures.
Catching parse errors and continue-ing hides upstream API format changes and can return partial/incorrect data without any signal.
Suggested fix
- except (ValueError, TypeError):
- continue
+ except (ValueError, TypeError) as exc:
+ raise ValueError(
+ f"Unexpected schedule date format for service '{bin_type}': {date_str!r}"
+ ) from excBased on learnings: "prefer explicit failures (raise exceptions on unexpected formats) over silent defaults or swallowed errors."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except (ValueError, TypeError): | |
| continue | |
| except (ValueError, TypeError) as exc: | |
| raise ValueError( | |
| f"Unexpected schedule date format for service '{bin_type}': {date_str!r}" | |
| ) from exc |
🤖 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/SheffieldCityCouncil.py` around
lines 94 - 95, In the schedule date parsing block inside the
SheffieldCityCouncil scraper (class SheffieldCityCouncil), stop silently
swallowing ValueError/TypeError; instead capture the original exception and
re-raise (or raise a new descriptive exception) including the offending date
string and context so upstream callers see the failure; if you have a logger
(e.g. self.logger or module logger) emit an error with the raw value before
raising to aid debugging.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2060 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 9 9
Lines 1141 1141
=======================================
Hits 989 989
Misses 152 152 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tests/input.json`:
- Line 2137: The "wiki_note" string for the entry currently mentions only "house
number" but the parser accepts house numbers or house names (PAON); update the
value of the "wiki_note" JSON field to explicitly say "house number or house
name (PAON)" and mention that UPRN is accepted but not required so the note
covers named properties as well; locate the JSON entry with the "wiki_note" key
(and related "house_number"/"PAON" semantics) and replace the wording
accordingly.
- Around line 2131-2133: The fixture currently includes both "uprn" and
"house_number", so _match_property() in SheffieldCityCouncil.py will always take
the UPRN branch and never exercise the postcode+house_number (paon) lookup; fix
by splitting into two fixtures (or duplicate this fixture): one retaining "uprn"
to test the UPRN path and a second that removes the "uprn" field but keeps
"postcode" and "house_number" to test the paon/postcode lookup, ensuring both
lookup branches are covered by tests.
🪄 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: e7099e49-1292-4c12-94bd-11433c002dfb
📒 Files selected for processing (1)
uk_bin_collection/tests/input.json
| "postcode": "S7 1RY", | ||
| "uprn": "100050931898", | ||
| "house_number": "22", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Locate Sheffield scraper and input fixture"
fd -i "SheffieldCityCouncil.py"
fd -i "input.json"
echo
echo "2) Inspect lookup precedence in Sheffield scraper"
rg -n -C3 --type=py '\buprn\b|\bpostcode\b|\bhouse_number\b|\bpaon\b' uk_bin_collection/uk_bin_collection/councils/SheffieldCityCouncil.py
echo
echo "3) Inspect how tests map fields from input.json into scraper arguments"
rg -n -C3 --type=py 'tests/input\.json|input\.json|uprn|postcode|house_number|paon' uk_bin_collectionRepository: robbrad/UKBinCollectionData
Length of output: 50384
🏁 Script executed:
rg -A5 -B5 'SheffieldCityCouncil' uk_bin_collection/tests/input.json | head -80Repository: robbrad/UKBinCollectionData
Length of output: 554
Separate fixtures needed to test both UPRN and postcode+house_number lookup paths.
With both uprn and house_number present, the _match_property() function checks uprn first (line 13 of SheffieldCityCouncil.py) and returns immediately on match, never reaching the paon matching logic. This fixture only exercises the UPRN path and doesn't test the postcode+house_number path. Split into two fixtures or remove uprn from this one to ensure coverage of both lookup methods.
🤖 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/tests/input.json` around lines 2131 - 2133, The fixture
currently includes both "uprn" and "house_number", so _match_property() in
SheffieldCityCouncil.py will always take the UPRN branch and never exercise the
postcode+house_number (paon) lookup; fix by splitting into two fixtures (or
duplicate this fixture): one retaining "uprn" to test the UPRN path and a second
that removes the "uprn" field but keeps "postcode" and "house_number" to test
the paon/postcode lookup, ensuring both lookup branches are covered by tests.
| "url": "https://wasteservices.sheffield.gov.uk", | ||
| "wiki_name": "Sheffield", | ||
| "wiki_note": "Follow the instructions [here](https://wasteservices.sheffield.gov.uk/) until you get the 'Your bin collection dates and services' page, then copy the URL and replace the URL in the command.", | ||
| "wiki_note": "Provide your postcode and house number. UPRN is also accepted but no longer required.", |
There was a problem hiding this comment.
Clarify that house_number also accepts house names (PAON).
Line 2137 currently says “house number”, but this parser now supports house number/name. Update the note to avoid excluding valid named properties.
✏️ Suggested wording update
- "wiki_note": "Provide your postcode and house number. UPRN is also accepted but no longer required.",
+ "wiki_note": "Provide your postcode and house number/name (PAON). UPRN is also accepted but no longer required.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "wiki_note": "Provide your postcode and house number. UPRN is also accepted but no longer required.", | |
| "wiki_note": "Provide your postcode and house number/name (PAON). UPRN is also accepted but no longer required.", |
🤖 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/tests/input.json` at line 2137, The "wiki_note" string for
the entry currently mentions only "house number" but the parser accepts house
numbers or house names (PAON); update the value of the "wiki_note" JSON field to
explicitly say "house number or house name (PAON)" and mention that UPRN is
accepted but not required so the note covers named properties as well; locate
the JSON entry with the "wiki_note" key (and related "house_number"/"PAON"
semantics) and replace the wording accordingly.
UPRN is no longer required. Users can provide postcode + house number as an alternative. Uses getPropertySearch API to match by address. Backward compatible - existing UPRN lookups still work.
InertiaUK
force-pushed
the
feat/sheffield-postcode-lookup
branch
from
924afd2 to
88dbdc7
Compare
1 participant
Summary
getPropertySearchAPI to search by postcode, then matches the property by house number/name.Testing
S7 1RY+ UPRN100050931898S7 1RY+ paon22Summary by CodeRabbit
Bug Fixes
Chores