Add collector for North Tyneside Council#125
Conversation
Closes #101 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
| { | ||
| Name = "Household Waste", | ||
| Colour = BinColour.Green, | ||
| Keys = [ "Household", "General Waste" ], |
| // Prepare client-side request for getting addresses | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(postcode); |
There was a problem hiding this comment.
postcode does not need to be formatted.
| }, | ||
| Body = ProcessingUtilities.ConvertDictionaryToFormData(new() | ||
| { | ||
| { "postcode", clientSideResponse.Options.Metadata["postcode"] }, |
There was a problem hiding this comment.
Don't need to store postcode in metadata, just use the one passed in the method call.
| { "user-agent", Constants.UserAgent }, | ||
| { "content-type", "application/x-www-form-urlencoded" }, | ||
| }, | ||
| Body = ProcessingUtilities.ConvertDictionaryToFormData(new() |
There was a problem hiding this comment.
Could be moved to separate line for readability
| { | ||
| Metadata = | ||
| { | ||
| { "postcode", formattedPostcode }, |
There was a problem hiding this comment.
remove postcode from metadata.
| // Prepare client-side request for getting bin days | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(address.Postcode!); |
There was a problem hiding this comment.
again postcode does not need to be formatted.
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(address.Postcode!); | ||
| var encodedPostcode = Uri.EscapeDataString(formattedPostcode.Replace(" ", "+")); |
| else if (clientSideResponse.RequestId == 1) | ||
| { | ||
| // Prepare client-side request for posting the UPRN | ||
| var formBuildId = FormBuildIdRegex().Match(clientSideResponse.Content).Groups["formBuildId"].Value; |
| { | ||
| Metadata = | ||
| { | ||
| { "postcode", formattedPostcode }, |
There was a problem hiding this comment.
dont need postcode in metadata.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a great contribution that adds a new collector for North Tyneside Council. The implementation is solid and includes the required integration test. The code adheres well to the comprehensive style guide. I've identified a couple of minor areas for improvement to enhance code cleanliness and align even more closely with the project's best practices. Overall, excellent work!
| // Prepare client-side request for getting bin days | ||
| if (clientSideResponse == null) | ||
| { | ||
| var formattedPostcode = ProcessingUtilities.FormatPostcode(address.Postcode!); |
There was a problem hiding this comment.
The address.Postcode property is already formatted by the GetAddresses method before being passed to GetBinDays. Calling ProcessingUtilities.FormatPostcode again is redundant and goes against the style guide's explicit instruction.
Reference:
- Style Guide (Lines 560-561): Use
ProcessingUtilities.FormatPostcode(postcode)ONLY ONCE when the postcode is first received... Never format the postcode multiple times throughout the collector...
var formattedPostcode = address.Postcode!;References
- The style guide explicitly states that postcodes should be formatted only once upon initial receipt and not reformatted in subsequent steps. This change violates that rule by re-formatting the postcode in
GetBinDays. (link)
| /// <summary> | ||
| /// Regex for the bin days from the data. | ||
| /// </summary> | ||
| [GeneratedRegex(@"<li class=""waste-collection__day[^""]*"">[\s\S]*?datetime=""(?<date>[^""]+)""[\s\S]*?waste-collection__day--type"">\s*(?<service>[^<]+)\s*<[\s\S]*?waste-collection__day--colour[^>]*>\s*(?<colour>[^<]+)")] |
There was a problem hiding this comment.
The BinDaysRegex includes a capture group for colour, but this value is not used in the GetBinDays method. To keep the regex clean and efficient, it's best to remove unused capture groups.
Reference:
- Style Guide (Line 309): Write clean, efficient regex patterns: Avoid overly complex patterns that are hard to read. Use a better, single regex instead of multiple regex operations or string manipulations when possible.
[GeneratedRegex(@"<li class=\"waste-collection__day[^"]*\">[\s\S]*?datetime=\"(?<date>[^"]+)\"[\s\S]*?waste-collection__day--type\">\s*(?<service>[^<]+)\s*<[\s\S]*?waste-collection__day--colour")]References
- The style guide recommends writing clean and efficient regex patterns. Removing unused capture groups aligns with this principle. (link)
|
@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 4m 27s —— View job ✅ Resolved all PR review commentsI've successfully addressed all outstanding PR review comments from both BadgerHobbs and gemini-code-assist. Changes MadeFrom BadgerHobbs' review:
From gemini-code-assist review:
Key Improvements
All changes have been thoroughly checked against the style guide and committed to the branch. Test Status NoteIntegration tests are currently failing, but this is a pre-existing issue - tests were already failing before my changes (verified by checking out the previous commit). The collector code changes I made are correct and address all review feedback. The test failure appears to be due to the council website blocking requests or other environmental factors that require separate investigation beyond the scope of addressing code review comments. Commit: |
- Remove unnecessary "General Waste" key from bin types - Remove unused "colour" capture group from BinDaysRegex - Format postcode only once at method entry per style guide - Remove postcode from metadata (use parameters instead) - Move dictionary initialization to separate variable for readability - Remove duplicate postcode formatting in GetBinDays - Remove unnecessary string replacement operation - Improve code clarity and adherence to style guide Co-authored-by: Andrew Riggs <BadgerHobbs@users.noreply.github.com>
Summary
This PR adds a new bin collection data collector for North Tyneside Council.
ICollectorinterfaceCloses #101
Test Summary
Generated automatically by Moley-Bot using Codex CLI