Handle end of year collection dates - West Suffolk #1788
Conversation
📝 WalkthroughWalkthroughModified date parsing logic in WestSuffolkCouncil.py to handle year transitions. When parsing bin collection dates, if the current month is December and the parsed month is January, the code now increments the year to the next year before formatting, replacing the previous approach of always using the current calendar year. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
uk_bin_collection/uk_bin_collection/councils/WestSuffolkCouncil.py
🧰 Additional context used
🪛 Ruff (0.14.10)
uk_bin_collection/uk_bin_collection/councils/WestSuffolkCouncil.py
66-66: datetime may be undefined, or defined from star imports
(F405)
68-68: datetime may be undefined, or defined from star imports
(F405)
| # if current month is December and next_collection month is January, set year to next year | ||
| year = datetime.datetime.now().year | ||
| if ( | ||
| datetime.datetime.now().month == 12 | ||
| and next_collection.month == 1 | ||
| ): | ||
| year += 1 | ||
| next_collection = next_collection.replace(year=year) |
There was a problem hiding this comment.
Address the datetime import and consider a more robust approach.
The year adjustment logic correctly handles December→January transitions. However, there are a few concerns:
-
Import issue: Static analysis correctly flags that
datetimemay be undefined due to the star import on line 6. While it likely works (assumingcommon.pyimports datetime), this is fragile and violates Python best practices. -
Multiple
datetime.now()calls: Lines 66 and 68 calldatetime.now()separately, which is inefficient and could theoretically cause edge-case issues if executed exactly at midnight. -
Narrow logic: The December→January check works for the stated problem, but a more general approach would be to check if the parsed date is in the past (using today's date with the assumed current year), and if so, increment the year. This would handle additional edge cases like November→January or queries run in late December that include early January dates.
🔎 Proposed improvements
Fix 1: Address the import and cache the current datetime
+ from datetime import datetime as dt
+
# Parse the date
next_collection = date_parse(collection_date)
# if current month is December and next_collection month is January, set year to next year
- year = datetime.datetime.now().year
+ now = dt.now()
+ year = now.year
if (
- datetime.datetime.now().month == 12
+ now.month == 12
and next_collection.month == 1
):
year += 1
next_collection = next_collection.replace(year=year)Fix 2 (alternative - more general approach): Check if date is in the past
+ from datetime import datetime as dt
+
# Parse the date
next_collection = date_parse(collection_date)
- # if current month is December and next_collection month is January, set year to next year
- year = datetime.datetime.now().year
- if (
- datetime.datetime.now().month == 12
- and next_collection.month == 1
- ):
- year += 1
- next_collection = next_collection.replace(year=year)
+ # Adjust year if the parsed date (with current year) is in the past
+ now = dt.now()
+ next_collection = next_collection.replace(year=now.year)
+ if next_collection.date() < now.date():
+ next_collection = next_collection.replace(year=now.year + 1)Fix 2 is more general and handles any case where the parsed date appears to be in the past.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
66-66: datetime may be undefined, or defined from star imports
(F405)
68-68: datetime may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In uk_bin_collection/uk_bin_collection/councils/WestSuffolkCouncil.py around
lines 65-72, replace the fragile star-import usage and duplicated datetime.now()
calls by explicitly importing the datetime module (or from datetime import
datetime) at the top, cache a single "today = datetime.now().date()" value, and
instead of the narrow December→January check set the parsed next_collection to
use the current year and if that date is before today increment the year by 1;
this removes multiple now() calls, fixes the undefined-import risk, and
generalizes the year-adjustment logic to handle any past-month parsed dates.
|
Just seen that #1787 has already been opened to address this. |
1 participant
Currently West Suffolk council does not display years for their bin collection days.
This causes next years dates to be in the past as it assumes all years are the current year.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.