Add collector for Bassetlaw District Council#187
Add collector for Bassetlaw District Council#187moley-bot[bot] wants to merge 3 commits intomainfrom
Conversation
Closes #184 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
| /// <summary> | ||
| /// The list of bin types for this collector. | ||
| /// </summary> | ||
| private readonly IReadOnlyCollection<Bin> _binTypes = |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new collector for Bassetlaw District Council. While the implementation correctly follows many project conventions, it has a critical flaw: it calculates future bin collection dates instead of parsing them directly from the data source, which is a direct violation of a core design principle outlined in the style guide. This must be refactored to ensure the collector is a faithful representation of the council's data. Additionally, there are several minor style and maintainability issues, such as missing trailing commas, an unused bin key, and a lack of documentation for the Uid format, which should also be addressed.
| var blueDates = BuildUpcomingDates(baseDates.Blue, collectionDay); | ||
| var greenDates = BuildUpcomingDates(baseDates.Green, collectionDay); | ||
|
|
||
| var binDays = new List<BinDay>(); | ||
|
|
||
| // Iterate through each general waste date, and create a new bin day object | ||
| foreach (var date in greenDates) | ||
| { | ||
| var binDay = new BinDay | ||
| { | ||
| Date = date, | ||
| Address = address, | ||
| Bins = ProcessingUtilities.GetMatchingBins(_binTypes, "Green"), | ||
| }; | ||
|
|
||
| binDays.Add(binDay); | ||
| } | ||
|
|
||
| // Iterate through each recycling date, and create a new bin day object | ||
| foreach (var date in blueDates) | ||
| { | ||
| var binDay = new BinDay | ||
| { | ||
| Date = date, | ||
| Address = address, | ||
| Bins = ProcessingUtilities.GetMatchingBins(_binTypes, "Blue"), | ||
| }; | ||
|
|
||
| binDays.Add(binDay); | ||
| } | ||
|
|
||
| var getBinDaysResponse = new GetBinDaysResponse | ||
| { | ||
| BinDays = ProcessingUtilities.ProcessBinDays(binDays), | ||
| }; | ||
|
|
||
| return getBinDaysResponse; | ||
| } | ||
|
|
||
| // Throw exception for invalid request | ||
| throw new InvalidOperationException("Invalid client-side request."); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a client-side request for a specific feature layer. | ||
| /// </summary> | ||
| private ClientSideRequest CreateLayerRequest(int requestId, int layerPosition, string x, string y) | ||
| { | ||
| var layerId = _layerOrder[layerPosition]; | ||
|
|
||
| var clientSideRequest = new ClientSideRequest | ||
| { | ||
| RequestId = requestId, | ||
| Url = $"https://services1.arcgis.com/P2LV4qXI9z8W2RdA/arcgis/rest/services/Bassetlaw_District_Bin_Collection_WFL1/FeatureServer/{layerId}/query?f=json&geometry={x},{y}&geometryType=esriGeometryPoint&inSR=27700&spatialRel=esriSpatialRelIntersects&outFields=*", | ||
| Method = "GET", | ||
| Headers = new() | ||
| { | ||
| { "user-agent", Constants.UserAgent }, | ||
| }, | ||
| Options = new ClientSideOptions | ||
| { | ||
| Metadata = | ||
| { | ||
| { "x", x }, | ||
| { "y", y }, | ||
| { "layerPosition", layerPosition.ToString(CultureInfo.InvariantCulture) }, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| return clientSideRequest; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines the collection day from the feature attributes. | ||
| /// </summary> | ||
| private static DayOfWeek GetCollectionDay(JsonObject attributes) | ||
| { | ||
| var dayFields = new (string Field, DayOfWeek Day)[] | ||
| { | ||
| ("Sunday", DayOfWeek.Sunday), | ||
| ("Monday", DayOfWeek.Monday), | ||
| ("Tuesday", DayOfWeek.Tuesday), | ||
| ("Wednesday", DayOfWeek.Wednesday), | ||
| ("Thursday", DayOfWeek.Thursday), | ||
| ("Friday", DayOfWeek.Friday), | ||
| ("Saturday", DayOfWeek.Saturday), | ||
| }; | ||
|
|
||
| foreach (var dayField in dayFields) | ||
| { | ||
| if (attributes.TryGetPropertyValue(dayField.Field, out var valueNode)) | ||
| { | ||
| var value = valueNode!.GetValue<string>().Trim(); | ||
|
|
||
| if (value.Equals("Yes", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return dayField.Day; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var dayName = attributes["Day_"]!.GetValue<string>().Trim(); | ||
|
|
||
| return Enum.Parse<DayOfWeek>(dayName); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Builds a list of upcoming collection dates using the fortnightly schedule. | ||
| /// </summary> | ||
| private static IReadOnlyCollection<DateOnly> BuildUpcomingDates(DateOnly baseDate, DayOfWeek collectionDay) | ||
| { | ||
| const int pickupWeekInterval = 2; | ||
| const int occurrences = 6; | ||
|
|
||
| var today = DateOnly.FromDateTime(DateTime.UtcNow); | ||
|
|
||
| var initialDate = GetNextCollectionDate( | ||
| today, | ||
| baseDate, | ||
| collectionDay, | ||
| pickupWeekInterval | ||
| ); | ||
|
|
||
| var dates = new List<DateOnly> | ||
| { | ||
| initialDate, | ||
| }; | ||
|
|
||
| for (var index = 1; index < occurrences; index++) | ||
| { | ||
| dates.Add(dates[index - 1].AddDays(pickupWeekInterval * 7)); | ||
| } | ||
|
|
||
| return dates; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Calculates the next collection date based on the base week and pickup interval. | ||
| /// </summary> | ||
| private static DateOnly GetNextCollectionDate( | ||
| DateOnly today, | ||
| DateOnly baseDate, | ||
| DayOfWeek collectionDay, | ||
| int pickupWeekInterval | ||
| ) | ||
| { | ||
| var todayWeekStart = today.AddDays(0 - (int)today.DayOfWeek); | ||
| var baseWeekStart = baseDate.AddDays(0 - (int)baseDate.DayOfWeek); | ||
|
|
||
| var weeksBetween = (todayWeekStart.ToDateTime(TimeOnly.MinValue) - baseWeekStart.ToDateTime(TimeOnly.MinValue)).Days / 7; | ||
| var weekRemainder = ((weeksBetween % pickupWeekInterval) + pickupWeekInterval) % pickupWeekInterval; | ||
|
|
||
| var daysUntilCollection = ((int)collectionDay - (int)today.DayOfWeek + 7) % 7; | ||
|
|
||
| if (weekRemainder > 0) | ||
| { | ||
| daysUntilCollection += 7 * (pickupWeekInterval - weekRemainder); | ||
| } | ||
| else if (daysUntilCollection == 0) | ||
| { | ||
| daysUntilCollection += pickupWeekInterval * 7; | ||
| } | ||
|
|
||
| return today.AddDays(daysUntilCollection); | ||
| } |
There was a problem hiding this comment.
This implementation calculates future bin collection dates using a base date and a fortnightly interval. This is a direct violation of a core design principle: collectors must only return collection dates explicitly provided by the council's API and should never calculate or infer them.
This "brittle by design" approach ensures that the data is a faithful representation of the source and that the collector fails loudly if the source logic changes, which is the desired behavior.
Please refactor this to parse collection dates directly from an API response, even if it requires additional client-side requests to mimic the user journey on the council website.
References
- Collectors must return ONLY the collection dates explicitly provided by the council website. Never calculate or infer additional dates based on intervals, patterns, or statements like "and every other week thereafter". The collector's responsibility is to faithfully represent the data source, not to project or compute future dates. (link)
| @@ -0,0 +1,347 @@ | |||
| namespace BinDays.Api.Collectors.Collectors.Councils; | |||
|
|
|||
| using BinDays.Api.Collectors.Collectors.Vendors; | |||
There was a problem hiding this comment.
This using statement for BinDays.Api.Collectors.Collectors.Vendors is not used within the file. According to the style guide, unused using statements should be removed to keep the code clean and reduce clutter.
References
- Unused
usingstatements should be removed to keep code clean and reduce clutter. (link)
| [ | ||
| new() | ||
| { | ||
| Name = "General Waste", | ||
| Colour = BinColour.Green, | ||
| Keys = [ "Green", "Custom" ], | ||
| }, | ||
| new() | ||
| { | ||
| Name = "Recycling", | ||
| Colour = BinColour.Blue, | ||
| Keys = [ "Blue" ], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
This collection initializer has a couple of style guide violations:
- Missing Trailing Commas: The items within the
new()blocks and the blocks themselves are missing trailing commas, which are required for cleaner diffs. - Unused Key: The
"Custom"key for 'General Waste' is not used for matching inGetBinDaysand should be removed.
Please update the initializer to include trailing commas and remove the unnecessary key.
[
new()
{
Name = "General Waste",
Colour = BinColour.Green,
Keys = [ "Green" ],
},
new()
{
Name = "Recycling",
Colour = BinColour.Blue,
Keys = [ "Blue" ],
},
];| private readonly Dictionary<int, (DateOnly Blue, DateOnly Green)> _baseDates = new() | ||
| { | ||
| { 0, (new DateOnly(2022, 1, 31), new DateOnly(2018, 1, 1)) }, | ||
| { 1, (new DateOnly(2022, 1, 25), new DateOnly(2022, 2, 1)) }, | ||
| { 3, (new DateOnly(2022, 2, 3), new DateOnly(2018, 1, 1)) }, | ||
| { 4, (new DateOnly(2022, 2, 4), new DateOnly(2022, 1, 28)) }, | ||
| { 22, (new DateOnly(2022, 2, 2), new DateOnly(2022, 1, 26)) }, | ||
| }; |
There was a problem hiding this comment.
This dictionary initializer is missing a trailing comma on the last entry. The style guide requires trailing commas in all multi-line initializers to improve diff clarity and reduce merge conflicts.
private readonly Dictionary<int, (DateOnly Blue, DateOnly Green)> _baseDates = new()
{
{ 0, (new DateOnly(2022, 1, 31), new DateOnly(2018, 1, 1)) },
{ 1, (new DateOnly(2022, 1, 25), new DateOnly(2022, 2, 1)) },
{ 3, (new DateOnly(2022, 2, 3), new DateOnly(2018, 1, 1)) },
{ 4, (new DateOnly(2022, 2, 4), new DateOnly(2022, 1, 28)) },
{ 22, (new DateOnly(2022, 2, 2), new DateOnly(2022, 1, 26)) },
};References
- Always use trailing commas in multi-line initializers to make future diffs cleaner and reduce merge conflicts. (link)
| var address = new Address | ||
| { | ||
| Property = property, | ||
| Postcode = postcode, | ||
| Uid = $"{uprn};{x};{y}", | ||
| }; |
There was a problem hiding this comment.
The Uid is being used to pass multiple data points (uprn, x, y) to the GetBinDays method, which is a great use of this pattern. However, the style guide recommends documenting the format of the concatenated Uid for clarity in both the method that creates it and the one that consumes it.
// Uid format: "uprn;x-coordinate;y-coordinate"
var address = new Address
{
Property = property,
Postcode = postcode,
Uid = $"{uprn};{x};{y}",
};References
- When concatenating multiple data parts into the UID, document the UID format with a comment in both methods (
GetAddressesandGetBinDays) for clarity. (link)
| // Prepare client-side request for getting bin polygons | ||
| if (clientSideResponse == null) | ||
| { | ||
| var coordinateParts = address.Uid!.Split(';'); |
There was a problem hiding this comment.
To improve clarity as per the style guide, please add a comment here documenting the expected format of the Uid that is being split.
// Uid format: "uprn;x-coordinate;y-coordinate"
var coordinateParts = address.Uid!.Split(';');References
- When splitting a concatenated UID, document the UID format with a comment for clarity. (link)
|
@claude Please fix all unresolved PR comments, making sure to test after each change made. Please include the test output in your final comment update. |
|
Claude finished @BadgerHobbs's task in 21m 50s —— View job PR Comment Fixes AppliedChanges committed:
Cannot fix — explained below: Garden Waste (BadgerHobbs @ line 29)After investigating the ArcGIS webmap app configuration, the webmap itself explicitly states:
There is no digital API for garden waste data — Bassetlaw handles garden waste subscribers entirely via paper/PDF calendars outside their GIS system. The Critical: Date Calculation (Gemini @ line 346)The Gemini review correctly identifies that the collector calculates dates rather than parsing them from an API response. However, this council does not have a date API. The Bassetlaw website uses ArcGIS Arcade expressions to compute collection dates entirely client-side using The FeatureServer stores only zone geometry and base reference dates (e.g. Trailing commas (Gemini @ line 43, 60)Already present — the Unused
|
- Remove unused "Custom" key from General Waste bin type - Add Uid format comments in GetAddresses and GetBinDays - Apply dotnet format improvements (tuple deconstruction) Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
1 participant
Summary
This PR adds a new bin collection data collector for Bassetlaw District Council.
ICollectorinterfaceCloses #184
Test Summary
Bin Collections Page Screenshot
Generated automatically by Moley-Bot using Codex CLI