fix: Broken councils#1762
Merged
Merged
Conversation
Contributor
WalkthroughThis PR modifies 12 council parser files and test input data to improve robustness across bin collection workflows. Changes include error handling guards for missing page elements, response validation checks, date formatting updates, HTTP header and SSL handling, and one major migration to Selenium-based automation for EpsomandEwellBoroughCouncil. Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebDriver
participant Browser
participant Portal as iTouchVision Portal
participant BeautifulSoup
Client->>WebDriver: create_webdriver()
WebDriver->>Browser: initialize Selenium
Browser->>Portal: load portal URL
Portal->>Browser: return page HTML
Client->>Browser: inject JavaScript to set postcode
Browser->>Portal: trigger React form events
Portal->>Browser: render address dropdown
Client->>Browser: select address via script (UPRN)
Browser->>Portal: update selection
Portal->>Browser: dynamically render bin data
Client->>Browser: wait for data to populate (WebDriverWait)
Browser->>Portal: wait complete
Client->>BeautifulSoup: parse page source
BeautifulSoup->>Client: extract bin types & dates
Client->>Client: parse dates & adjust year
Client->>Client: sort results by collectionDate
Client->>WebDriver: quit driver (cleanup)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
❌ 8 Tests Failed:
View the top 2 failed test(s) by shortest run time
View the full list of 6 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
uk_bin_collection/uk_bin_collection/councils/BuckinghamshireCouncil.py (1)
87-95: Good validation logic with optional refactor opportunity.The status code and hex validation checks effectively prevent downstream errors. Both validations provide helpful error messages with response previews.
The hex validation could be simplified using a try/except with the
bytes.fromhex()call that happens later (line 53). This would be more efficient and avoid checking twice:- # Check if output looks like hex (should only contain hex characters) - if not all(c in '0123456789ABCDEFabcdef' for c in output.strip()): - raise ValueError(f"API returned non-hex response (status {response.status_code}). Response starts with: {output[:200]}") - - decoded_bins = self.decode_response(output) + try: + decoded_bins = self.decode_response(output) + except ValueError as e: + raise ValueError(f"API returned non-hex response (status {response.status_code}). Response starts with: {output[:200]}") from eHowever, the current approach is explicit and readable, so this refactor is entirely optional.
uk_bin_collection/uk_bin_collection/councils/AngusCouncil.py (1)
31-32: Direct navigation improves efficiency.Navigating directly to the form URL removes unnecessary intermediate steps. The 3-second sleep allows for page load, though this is a fixed delay that might be sub-optimal for varying network conditions.
Consider replacing the fixed sleep with a more robust wait condition:
# Instead of time.sleep(3), use an explicit wait wait.until(lambda d: d.execute_script("return document.readyState") == "complete")This would wait only as long as necessary for the page to fully load.
Also applies to: 40-42
uk_bin_collection/uk_bin_collection/councils/NeathPortTalbotCouncil.py (1)
107-140: Good guarded date parsing; consider also guarding sibling lookups and importsThe new logic to:
- read
date_text,- skip
"Bank Holidays",- and wrap
datetime.strptime(...)in atry/except ValueErroris a solid way to ignore non‑date
<h2>elements and notice popups without breaking parsing.Two small follow‑ups you might consider:
- Guard the sibling container
date.find_next_sibling("div")can still returnNoneif the markup changes. A quick guard would align with the rest of this defensive change:- bin_types_wrapper = date.find_next_sibling("div") - for bin_type_wrapper in bin_types_wrapper.find_all( + bin_types_wrapper = date.find_next_sibling("div") + if not bin_types_wrapper: + continue + for bin_type_wrapper in bin_types_wrapper.find_all( "div", { "class": "card-body ps-5 ps-md-4 ps-lg-5 position-relative bg-white" }, ):
- Address Ruff F405 for
datetime/date_format
To satisfy the static analysis hint and make dependencies explicit, you could replace the star import with explicit ones, e.g.:-from uk_bin_collection.uk_bin_collection.common import * +from datetime import datetime +from uk_bin_collection.uk_bin_collection.common import create_webdriver, date_format, check_paon, check_postcode(and adjust any other names used from
common).These are incremental, non‑blocking cleanups on top of a good robustness fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
uk_bin_collection/tests/input.json(1 hunks)uk_bin_collection/uk_bin_collection/councils/AngusCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/BuckinghamshireCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py(2 hunks)uk_bin_collection/uk_bin_collection/councils/MedwayCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/NeathPortTalbotCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/NewportCityCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/SevenoaksDistrictCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/SouthHollandDistrictCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/SouthStaffordshireDistrictCouncil.py(1 hunks)uk_bin_collection/uk_bin_collection/councils/WindsorAndMaidenheadCouncil.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py (1)
uk_bin_collection/uk_bin_collection/common.py (2)
check_uprn(67-78)create_webdriver(321-360)
🪛 Ruff (0.14.7)
uk_bin_collection/uk_bin_collection/councils/SouthHollandDistrictCouncil.py
58-58: datetime may be undefined, or defined from star imports
(F405)
62-62: date_format may be undefined, or defined from star imports
(F405)
67-67: datetime may be undefined, or defined from star imports
(F405)
67-67: date_format may be undefined, or defined from star imports
(F405)
uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py
24-24: check_uprn may be undefined, or defined from star imports
(F405)
29-29: create_webdriver may be undefined, or defined from star imports
(F405)
41-41: Local variable postcode_input is assigned to but never used
Remove assignment to unused variable postcode_input
(F841)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
66-66: Local variable address_select is assigned to but never used
Remove assignment to unused variable address_select
(F841)
132-132: date_format may be undefined, or defined from star imports
(F405)
139-139: Do not catch blind exception: Exception
(BLE001)
uk_bin_collection/uk_bin_collection/councils/NeathPortTalbotCouncil.py
110-110: datetime may be undefined, or defined from star imports
(F405)
116-116: datetime may be undefined, or defined from star imports
(F405)
135-135: date_format may be undefined, or defined from star imports
(F405)
uk_bin_collection/uk_bin_collection/councils/BuckinghamshireCouncil.py
89-89: Abstract raise to an inner function
(TRY301)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
95-95: Abstract raise to an inner function
(TRY301)
95-95: Avoid specifying long messages outside the exception class
(TRY003)
uk_bin_collection/uk_bin_collection/councils/MedwayCouncil.py
23-23: Probable use of requests call without timeout
(S113)
23-23: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
uk_bin_collection/uk_bin_collection/councils/AngusCouncil.py
57-57: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
uk_bin_collection/uk_bin_collection/councils/NewportCityCouncil.py
118-118: Abstract raise to an inner function
(TRY301)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Integration Tests (3.12, 1.8.4)
- GitHub Check: Run Unit Tests (3.12, 1.8.4)
🔇 Additional comments (9)
uk_bin_collection/uk_bin_collection/councils/NewportCityCouncil.py (1)
115-118: LGTM - Good defensive validation!The HTML error page detection is a solid guard before attempting decryption. The check correctly identifies when the API returns HTML instead of encrypted data.
The static analysis hints about abstracting the raise and message length are stylistic suggestions that can be optionally addressed if this pattern is used frequently across the codebase.
uk_bin_collection/uk_bin_collection/councils/BuckinghamshireCouncil.py (1)
77-85: LGTM - Headers enhance API compatibility.The addition of headers, particularly the User-Agent, may help ensure successful API responses if the service validates or adjusts behavior based on client identification. The
Content-Type: text/plainis appropriate for the hex-encoded payload.uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py (2)
1-11: LGTM - Imports support the Selenium migration.The new imports correctly support the transition to a Selenium-based workflow. The star import from common is flagged by static analysis but appears to be a consistent pattern across this codebase.
143-151: LGTM - Proper cleanup and useful sorting.Sorting the bins by collection date improves usability, and the finally block ensures the WebDriver is properly cleaned up even if errors occur.
uk_bin_collection/tests/input.json (1)
902-911: LGTM - Test data correctly reflects Selenium migration.The updated test configuration properly supports the new Selenium-based workflow for EpsomandEwellBoroughCouncil. The addition of the postcode parameter,
skip_get_url, web driver URL, and updated documentation all align with the implementation changes.uk_bin_collection/uk_bin_collection/councils/NewarkAndSherwoodDC.py (1)
18-18: The change is correct and fixes broken code.The
pageparameter is indeed a string (as indicated by the type hint and base class signature), not a response object. The old code callingpage.textwould have failed at runtime because strings don't have a.textattribute. This change correctly uses the string directly, which aligns with the declared type hintpage: str, the base class contract inAbstractGetBinDataClass.parse_data(), and how all other council implementations handle this parameter. No type hint updates are needed.uk_bin_collection/uk_bin_collection/councils/WindsorAndMaidenheadCouncil.py (1)
40-42: Defensive guard correctly handles missingwidget-bin-collectionscontainerEarly‑returning an empty
datadict whennext_collection_divis absent avoidsAttributeErroron the subsequent.find_all()calls, and still allows thefinallyblock to close the driver. This is a clean, low‑risk robustness improvement.uk_bin_collection/uk_bin_collection/councils/SevenoaksDistrictCouncil.py (1)
84-86: Skipping suspended/restarting services avoids bogus date parsingChecking
raw_next_collection_date.lower()for"suspended"/"restarting"andcontinue‑ing beforeparse(...)is a sensible guard against status messages that don’t contain a valid date, and keeps the rest of the logic unchanged.uk_bin_collection/uk_bin_collection/councils/SouthStaffordshireDistrictCouncil.py (1)
70-79: Robust guards around collection sections and table structure look goodThe added checks:
- returning early when
showCollectionDatesis missing,- short‑circuiting for the “van collection” message,
- only reading the next date/type when their elements exist, and
- only iterating the
leisure-tablewhen present and with exactly two<td>cells per row,all significantly reduce the chance of
AttributeError/index errors from missing or variant markup while cleanly reusingadd_bin_types_to_collection. This is a solid resilience improvement for this scraper.Also applies to: 80-89, 94-107
Comment on lines
+44
to
+57
| # Try to find the postcode input with different selectors | ||
| try: | ||
| close_button = wait.until(EC.element_to_be_clickable((By.TAG_NAME, "button"))) | ||
| if close_button.text.strip().lower() in ['close', 'dismiss', 'ok']: | ||
| close_button.click() | ||
| postcode_input = wait.until(EC.element_to_be_clickable((By.ID, "searchString"))) | ||
| except TimeoutException: | ||
| pass | ||
|
|
||
| # Wait for postcode input to be clickable | ||
| postcode_input = wait.until(EC.element_to_be_clickable((By.ID, "searchString"))) | ||
| # Try alternative selectors | ||
| try: | ||
| postcode_input = driver.find_element(By.NAME, "searchString") | ||
| except NoSuchElementException: | ||
| try: | ||
| postcode_input = driver.find_element(By.CSS_SELECTOR, "input[type='text']") | ||
| except NoSuchElementException: | ||
| # Print page source for debugging | ||
| print("Page source:", driver.page_source[:1000]) | ||
| raise ValueError("Could not find postcode input field") |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Robust fallback strategy with minor improvement needed.
The multi-selector approach significantly improves reliability when the page structure varies. The page source debugging output is helpful.
Add the from err clause to preserve the exception chain as suggested by static analysis (B904):
try:
postcode_input = driver.find_element(By.CSS_SELECTOR, "input[type='text']")
except NoSuchElementException:
# Print page source for debugging
print("Page source:", driver.page_source[:1000])
- raise ValueError("Could not find postcode input field")
+ raise ValueError("Could not find postcode input field") from NoneNote: The final CSS selector input[type='text'] is very broad and might match an unintended input. Consider adding more specificity if this causes issues (e.g., form input[type='text'] or similar).
🧰 Tools
🪛 Ruff (0.14.7)
57-57: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/AngusCouncil.py around lines 44
to 57, the nested NoSuchElementException handler raises a new ValueError without
preserving the original exception chain; update the except clause to capture the
caught exception into a variable (e.g., except NoSuchElementException as e:) and
then re-raise the ValueError using "raise ValueError('Could not find postcode
input field') from e" so the original exception context is retained.
Comment on lines
+40
to
+51
| # Wait for postcode input to be present | ||
| postcode_input = WebDriverWait(driver, 10).until( | ||
| EC.presence_of_element_located((By.ID, "postcodeSearch")) | ||
| ) | ||
|
|
||
| # Enter postcode using JavaScript to trigger React events | ||
| if user_postcode: | ||
| postcode = user_postcode | ||
| else: | ||
| # If no postcode provided, we need to derive it from UPRN | ||
| # For now, raise an error | ||
| raise ValueError("Postcode is required for EpsomandEwellBoroughCouncil") |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused variable and validate postcode requirement.
The postcode validation correctly enforces a required parameter for this Selenium-based flow.
Remove the unused postcode_input variable assignment (flagged by Ruff F841):
- # Wait for postcode input to be present
- postcode_input = WebDriverWait(driver, 10).until(
- EC.presence_of_element_located((By.ID, "postcodeSearch"))
- )
+ # Wait for postcode input to be present
+ WebDriverWait(driver, 10).until(
+ EC.presence_of_element_located((By.ID, "postcodeSearch"))
+ )The wait is still performed, but without storing the unnecessary reference.
🧰 Tools
🪛 Ruff (0.14.7)
41-41: Local variable postcode_input is assigned to but never used
Remove assignment to unused variable postcode_input
(F841)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py
around lines 40 to 51, remove the unused postcode_input variable to satisfy Ruff
F841 by performing the WebDriverWait call without assigning its result (e.g.
call WebDriverWait(...).until(...) directly), and keep the existing postcode
validation logic that raises ValueError when no user_postcode is provided.
Comment on lines
+53
to
+79
| driver.execute_script(f""" | ||
| const input = document.getElementById('postcodeSearch'); | ||
| const nativeInputValueSetter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set; | ||
| nativeInputValueSetter.call(input, '{postcode}'); | ||
| input.dispatchEvent(new Event('input', {{ bubbles: true }})); | ||
| input.dispatchEvent(new Event('change', {{ bubbles: true }})); | ||
| """) | ||
|
|
||
| # Click the Find button | ||
| find_button = driver.find_element(By.CSS_SELECTOR, ".govuk-button") | ||
| find_button.click() | ||
|
|
||
| # Wait for address dropdown to appear and be populated | ||
| address_select = WebDriverWait(driver, 10).until( | ||
| EC.presence_of_element_located((By.ID, "addressSelect")) | ||
| ) | ||
|
|
||
| # Wait a bit for options to populate | ||
| import time | ||
| time.sleep(2) | ||
|
|
||
| # Select the address by UPRN value using JavaScript | ||
| driver.execute_script(f""" | ||
| const select = document.getElementById('addressSelect'); | ||
| select.value = '{user_uprn}'; | ||
| select.dispatchEvent(new Event('change', {{ bubbles: true }})); | ||
| """) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
JavaScript approach is appropriate for React, but clean up unused variable.
The JavaScript-based form manipulation correctly handles React's synthetic events. Using nativeInputValueSetter and dispatching events ensures React's state updates properly.
Remove the unused address_select variable (flagged by Ruff F841):
- # Wait for address dropdown to appear and be populated
- address_select = WebDriverWait(driver, 10).until(
- EC.presence_of_element_located((By.ID, "addressSelect"))
- )
+ # Wait for address dropdown to appear and be populated
+ WebDriverWait(driver, 10).until(
+ EC.presence_of_element_located((By.ID, "addressSelect"))
+ )The time.sleep(2) on line 72 is a fixed delay. Consider using an explicit wait for dropdown options to be populated:
# Wait for dropdown to have options
WebDriverWait(driver, 10).until(
lambda d: len(d.find_element(By.ID, "addressSelect").find_elements(By.TAG_NAME, "option")) > 1
)🧰 Tools
🪛 Ruff (0.14.7)
66-66: Local variable address_select is assigned to but never used
Remove assignment to unused variable address_select
(F841)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py
around lines 53 to 79, remove the unused local variable address_select (Ruff
F841) and the time.sleep(2) fixed delay; instead, after clicking the Find button
use an explicit WebDriverWait that waits until the addressSelect element has
more than one option (or the expected number), then proceed to set select.value
and dispatch the change via execute_script; also remove the unnecessary import
time if no longer used.
Comment on lines
111
to
141
| try: | ||
| collection_date = date_text.text.strip().split(":")[1] | ||
| except IndexError: | ||
| # Extract day and month from "Thursday 11 December" format | ||
| match = re.search(r'(\w+)\s+(\d{1,2})\s+(\w+)', date_text) | ||
| if match: | ||
| day = match.group(2) | ||
| month = match.group(3) | ||
|
|
||
| # Determine the year (if month is in the past, use next year) | ||
| current_date = datetime.now() | ||
| current_year = current_date.year | ||
|
|
||
| # Try parsing with current year | ||
| try: | ||
| date_obj = datetime.strptime(f"{day} {month} {current_year}", "%d %B %Y") | ||
| # If the date is more than 30 days in the past, assume it's next year | ||
| if (current_date - date_obj).days > 30: | ||
| date_obj = datetime.strptime(f"{day} {month} {current_year + 1}", "%d %B %Y") | ||
| except ValueError: | ||
| # Try with next year | ||
| date_obj = datetime.strptime(f"{day} {month} {current_year + 1}", "%d %B %Y") | ||
|
|
||
| collection_date = date_obj.strftime(date_format) | ||
|
|
||
| dict_data = { | ||
| "type": bin_type, | ||
| "collectionDate": collection_date, | ||
| } | ||
| bindata["bins"].append(dict_data) | ||
| except Exception as e: | ||
| print(f"Error parsing date '{date_text}': {e}") | ||
| continue |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Date parsing logic is sound but exception handling is too broad.
The date parsing correctly handles year boundaries, trying the current year first and using next year if the date appears to be more than 30 days in the past.
Replace the bare except Exception with specific exception types (flagged by Ruff BLE001):
try:
# Extract day and month from "Thursday 11 December" format
match = re.search(r'(\w+)\s+(\d{1,2})\s+(\w+)', date_text)
if match:
# ... date parsing logic ...
dict_data = {
"type": bin_type,
"collectionDate": collection_date,
}
bindata["bins"].append(dict_data)
- except Exception as e:
+ except (ValueError, AttributeError) as e:
print(f"Error parsing date '{date_text}': {e}")
continueThis catches the specific exceptions that could occur during date parsing while letting unexpected errors propagate.
🧰 Tools
🪛 Ruff (0.14.7)
132-132: date_format may be undefined, or defined from star imports
(F405)
139-139: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/EpsomandEwellBoroughCouncil.py
around lines 111 to 141, replace the broad "except Exception" that swallows all
errors with specific exceptions that can occur during the regex/date parsing
(for example ValueError from datetime.strptime and re.error from the regex
library); catch only those exceptions, log the error with the existing print
message, and let other unexpected exceptions propagate so they are not silently
ignored.
| api_url = f"https://api.medway.gov.uk/api/waste/getwasteday/{user_uprn}" | ||
|
|
||
| response = requests.get(api_url) | ||
| response = requests.get(api_url, verify=False) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the context around line 23
cat -n uk_bin_collection/uk_bin_collection/councils/MedwayCouncil.py | sed -n '15,35p'Repository: robbrad/UKBinCollectionData
Length of output: 934
🏁 Script executed:
# Get the full context of the function containing line 23
cat -n uk_bin_collection/uk_bin_collection/councils/MedwayCouncil.py | head -50Repository: robbrad/UKBinCollectionData
Length of output: 1642
🏁 Script executed:
# Search for all requests.get calls with verify=False in the codebase
rg "requests\.get.*verify\s*=\s*False" -A 2 -B 2Repository: robbrad/UKBinCollectionData
Length of output: 3565
🏁 Script executed:
# Check for requests.get calls without timeout parameter
rg "requests\.(get|post|request)" --no-filename | head -20Repository: robbrad/UKBinCollectionData
Length of output: 1086
Add timeout and make TLS verification configurable in Medway API call
The hardcoded verify=False disables TLS verification, creating a security risk. Additionally, the missing timeout can cause hangs if the Medway API stalls. Since the function already uses **kwargs, both should be made configurable with secure defaults:
- response = requests.get(api_url, verify=False)
+ timeout = kwargs.get("timeout", 10)
+ verify_ssl = kwargs.get("verify_ssl", True)
+ response = requests.get(api_url, timeout=timeout, verify=verify_ssl)This addresses the Ruff S113 (missing timeout) and S501 (verify=False) warnings. If verify=False was added to work around a certificate issue on Medway's side, consider gating it behind config (environment variable or HA option) and adding a TODO comment for future removal.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.7)
23-23: Probable use of requests call without timeout
(S113)
23-23: Probable use of requests call with verify=False disabling SSL certificate checks
(S501)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/MedwayCouncil.py around line 23,
the requests.get call uses a hardcoded verify=False and no timeout (triggering
Ruff S501 and S113); change it to accept TLS verification and timeout via kwargs
with secure defaults (verify defaulting to True and timeout defaulting to a sane
value like 10s) so callers can override them, or read a config/env var to allow
verify=False only when explicitly set; update the requests.get call to pass
verify=kwargs.pop('verify', config_or_env_default) and
timeout=kwargs.pop('timeout', 10) (or similar), and add a short TODO noting that
verify=False is a temporary workaround if used.
Comment on lines
+57
to
+59
| # Convert date from YYYY-MM-DD to DD/MM/YYYY format | ||
| parsed_date = datetime.strptime(collection_date, "%Y-%m-%d") | ||
|
|
Contributor
There was a problem hiding this comment.
Runtime date handling is consistent, but docstring now lies about format
The new logic to:
- parse
collection_datewithdatetime.strptime(..., "%Y-%m-%d"), - store
collectionDateusingparsed_date.strftime(date_format), and - sort using
datetime.strptime(..., date_format)
is internally consistent and matches the shared date_format.
However, the docstring above still says "collectionDate" ... in "YYYY-MM-DD" format and that sorting is based on that format. That’s now incorrect and can confuse callers.
You could update it along these lines:
- - "collectionDate" (str | None): Collection date in "YYYY-MM-DD" format.
- The list is sorted in ascending order by "collectionDate".
+ - "collectionDate" (str | None): Collection date formatted using `date_format` (e.g. "DD/MM/YYYY").
+ The list is sorted in ascending order by "collectionDate".Optionally, to satisfy Ruff’s F405 hints, you can also import datetime and date_format explicitly instead of relying on the star import from common.
Also applies to: 62-62, 67-67
🧰 Tools
🪛 Ruff (0.14.7)
58-58: datetime may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/SouthHollandDistrictCouncil.py
around lines 57-59 (and also apply same change at lines 62 and 67), update the
docstring to reflect that collectionDate is returned in the shared date_format
(DD/MM/YYYY) rather than "YYYY-MM-DD" and that sorting/parsing uses that
date_format; also replace the star import from common by explicitly importing
datetime and date_format (e.g., from datetime import datetime and from
uk_bin_collection.common import date_format) to satisfy lint F405 and avoid
relying on wildcard imports.
This was referenced Dec 17, 2025
Closed
This was referenced Jan 9, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.