Add collector for London Borough of Enfield#127
Conversation
Closes #100 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
| { | ||
| Name = "General Waste", | ||
| Colour = BinColour.Black, | ||
| Keys = [ "Residual", "Refuse", "General" ], |
There was a problem hiding this comment.
Are all these keys required?
| RequestId = 1, | ||
| Url = "https://www.enfield.gov.uk/services/rubbish-and-recycling/find-my-collection-day", | ||
| Method = "GET", | ||
| Headers = new() |
There was a problem hiding this comment.
Do we need all these headers?
| // Prepare client-side request for getting addresses | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(postcode); |
There was a problem hiding this comment.
Don't need to format postcode, already formatted.
| { | ||
| Metadata = | ||
| { | ||
| { "postcode", formattedPostcode }, |
There was a problem hiding this comment.
Don't need to include postcode in metadata.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new collector for the London Borough of Enfield, complete with integration tests. The implementation correctly follows the project's multi-step request pattern for fetching data. My review focuses on ensuring adherence to the repository's C# style guide. I've identified several minor violations, such as an unused using statement, missing trailing commas in object initializers, and incorrect formatting for a method call. Additionally, I've suggested reducing the number of browser-specific HTTP headers to create a more robust and minimal implementation, and recommended a safer way to access response headers as per the style guide.
| { "user-agent", Constants.UserAgent }, | ||
| { "accept", "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8" }, | ||
| { "accept-language", "en-US,en;q=0.9" }, | ||
| { "sec-ch-ua", "\"Not A(Brand\";v=\"99\", \"Mozilla\";v=\"137\", \"Chromium\";v=\"137\"" }, | ||
| { "sec-ch-ua-mobile", "?0" }, | ||
| { "sec-ch-ua-platform", "\"Windows\"" }, | ||
| { "upgrade-insecure-requests", "1" }, |
There was a problem hiding this comment.
The style guide encourages minimal request payloads (lines 228-235) and questioning whether every field is required. Many of these headers (e.g., accept-language, sec-ch-ua, sec-ch-ua-mobile, sec-ch-ua-platform, upgrade-insecure-requests) are browser-specific and may not be necessary for the API call to succeed. Please test with a minimal set of headers (likely just user-agent and accept) to keep the collector lightweight and less brittle. This applies to all requests in this collector.
References
- Collectors should not use browser emulation tools. While this isn't using a tool, sending excessive browser-specific headers goes against the principle of keeping collectors lightweight. (link)
- Request bodies should be minimal. This principle extends to headers, where every field should be questioned for its necessity. (link)
| @@ -0,0 +1,280 @@ | |||
| namespace BinDays.Api.Collectors.Collectors.Councils; | |||
|
|
|||
| using BinDays.Api.Collectors.Collectors.Vendors; | |||
| }, | ||
| Options = new ClientSideOptions | ||
| { | ||
| Metadata = | ||
| { | ||
| { "postcode", formattedPostcode }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The style guide (line 501) requires trailing commas after every property in multi-line initializers. The Headers and Options properties are missing trailing commas. Please add them for consistency. This issue is present in other ClientSideRequest initializers in this file as well (lines 122, 129, 194, 229).
},
Options = new ClientSideOptions
{
Metadata =
{
{ "postcode", formattedPostcode },
},
},References
- Always use trailing commas after every property in multi-line initializers. (link)
BinDays.Api.Collectors/Collectors/Councils/LondonBoroughOfEnfield.cs
Outdated
Show resolved
Hide resolved
| var parsedDate = DateTime.ParseExact( | ||
| scheduledStart, | ||
| "yyyy-MM-dd'T'HH:mm:ss", | ||
| CultureInfo.InvariantCulture, | ||
| DateTimeStyles.None | ||
| ); |
There was a problem hiding this comment.
According to the style guide (lines 501, 503, 528), multi-line method calls should use trailing commas for each argument, and the closing parenthesis should be on its own line.
var parsedDate = DateTime.ParseExact(
scheduledStart,
"yyyy-MM-dd'T'HH:mm:ss",
CultureInfo.InvariantCulture,
DateTimeStyles.None,
);|
@claude Please resolve all outstanding PR comments, be thourough, and test the changes you make and check against the style guide. |
|
Claude finished @BadgerHobbs's task in 8m 2s —— View job Resolved PR Review CommentsI've addressed all outstanding review comments and tested the changes thoroughly. Changes Made
Regarding HTTP HeadersAfter thorough testing, I found that minimizing headers as suggested causes the Enfield API to return HTML error pages instead of JSON, indicating anti-bot detection. The Test ResultsAll integration tests pass with the applied changes: | |
- Remove unnecessary keys from bin types (keep only "Residual" and "Food") - Update header access to use TryGetValue pattern - Remove redundant postcode formatting and metadata storage - Tests passing Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
Summary
This PR adds a new bin collection data collector for London Borough of Enfield.
ICollectorinterfaceCloses #100
Test Summary
Generated automatically by Moley-Bot using Codex CLI