Add collector for London Borough of Newham#134
Conversation
Closes #128 Generated with Codex CLI by Moley-Bot
Formatted by Moley-Bot
|
/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 the London Borough of Newham, complete with integration tests. The implementation is well-structured and correctly follows the multi-step request pattern. My review includes several minor suggestions to better align the code with the repository's style guide, focusing on removing an unused using statement, adding missing trailing commas, simplifying a regular expression, and ensuring addresses are returned in their original API order.
| @@ -0,0 +1,275 @@ | |||
| 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. Removing it will help keep the code clean and reduce clutter.
References
- The style guide advises removing unused
usingstatements to keep code clean and reduce clutter. (link)
| [GeneratedRegex(@"<a[^>]+/Details/Index/(?<uid>[^""]+)""[^>]*>Select</a></td>\s*<td>(?<line1>[^<]*)</td>\s*<td>(?<line2>[^<]*)</td>\s*<td>(?<postcode>[^<]+)</td>", RegexOptions.IgnoreCase | RegexOptions.Singleline)] | ||
| private static partial Regex AddressRegex(); |
There was a problem hiding this comment.
The style guide recommends using the postcode parameter directly rather than extracting it from other data. This regex includes a capture group for the postcode (?<postcode>) which is not used. Removing this unnecessary capture group simplifies the expression.
[GeneratedRegex(@"<a[^>]+/Details/Index/(?<uid>[^"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'"'d.Index/(?<uid>[^\"\"]+)\"[^>]*>Select</a></td>\s*<td>(?<line1>[^<]*)</td>\s*<td>(?<line2>[^<]*)</td>\s*<td>[^<]+</td>", RegexOptions.IgnoreCase | RegexOptions.Singleline)]References
- The style guide recommends using the
postcodeparameter directly instead of parsing it from address data. It also advises removing unused regex methods, which can be extended to removing unused capture groups within a regex. (link)
| var requestBody = ProcessingUtilities.ConvertDictionaryToFormData(new() | ||
| { | ||
| { "Address", postcode }, | ||
| { "btnSearch", "Search" }, | ||
| { "as_sfid", asSfid }, | ||
| { "as_fid", asFid }, | ||
| }); |
There was a problem hiding this comment.
The project's style guide requires trailing commas in multi-line initializers to improve diff clarity. Please add a trailing comma to the dictionary being created here.
var requestBody = ProcessingUtilities.ConvertDictionaryToFormData(new()
{
{ "Address", postcode },
{ "btnSearch", "Search" },
{ "as_sfid", asSfid },
{ "as_fid", asFid },
});References
- The style guide mandates the use of trailing commas in all multi-line initializers to produce cleaner version control diffs. (link)
| Dictionary<string, string> requestHeaders = new() | ||
| { | ||
| { "user-agent", Constants.UserAgent }, | ||
| { "content-type", "application/x-www-form-urlencoded" }, | ||
| { "cookie", cookie }, | ||
| }; |
There was a problem hiding this comment.
To align with the project's style guide, please add a trailing comma to this multi-line dictionary initializer. This practice helps maintain clean diffs in version control.
Dictionary<string, string> requestHeaders = new()
{
{ "user-agent", Constants.UserAgent },
{ "content-type", "application/x-www-form-urlencoded" },
{ "cookie", cookie },
};References
- The style guide mandates the use of trailing commas in all multi-line initializers to produce cleaner version control diffs. (link)
| var numericAddresses = new List<Address>(); | ||
| var otherAddresses = new List<Address>(); | ||
| foreach (Match rawAddress in rawAddresses) | ||
| { | ||
| var line1 = rawAddress.Groups["line1"].Value.Trim(); | ||
| var line2 = rawAddress.Groups["line2"].Value.Trim(); | ||
| var property = string.Join( | ||
| ", ", | ||
| new[] { line1, line2 }.Where(value => !string.IsNullOrWhiteSpace(value)) | ||
| ); | ||
|
|
||
| var address = new Address | ||
| { | ||
| Property = property, | ||
| Postcode = postcode, | ||
| Uid = rawAddress.Groups["uid"].Value, | ||
| }; | ||
|
|
||
| var hasLeadingDigit = !string.IsNullOrWhiteSpace(line1) && char.IsDigit(line1[0]); | ||
|
|
||
| if (hasLeadingDigit) | ||
| { | ||
| numericAddresses.Add(address); | ||
| } | ||
| else | ||
| { | ||
| otherAddresses.Add(address); | ||
| } | ||
| } | ||
|
|
||
| var getAddressesResponse = new GetAddressesResponse | ||
| { | ||
| Addresses = [.. numericAddresses, .. otherAddresses], | ||
| }; |
There was a problem hiding this comment.
The style guide specifies that addresses should be returned in the order received from the API, without any sorting. This logic separates addresses into numeric and non-numeric lists, altering the original order. The implementation should be simplified to process and return addresses in the sequence they are parsed.
// Iterate through each address, and create a new address object
var addresses = new List<Address>();
foreach (Match rawAddress in rawAddresses)
{
var line1 = rawAddress.Groups["line1"].Value.Trim();
var line2 = rawAddress.Groups["line2"].Value.Trim();
var property = string.Join(
", ",
new[] { line1, line2 }.Where(value => !string.IsNullOrWhiteSpace(value))
);
var address = new Address
{
Property = property,
Postcode = postcode,
Uid = rawAddress.Groups["uid"].Value,
};
addresses.Add(address);
}
var getAddressesResponse = new GetAddressesResponse
{
Addresses = [.. addresses],
};References
- The style guide explicitly states not to sort addresses and to return them in the order they are received from the API. (link)
| }, | ||
| ]; | ||
|
|
||
| private const string _baseUrl = "https://bincollection.newham.gov.uk/"; |
| // Prepare client-side request for searching addresses | ||
| else if (clientSideResponse.RequestId == 1) | ||
| { | ||
| clientSideResponse.Headers.TryGetValue("set-cookie", out var setCookieHeader); |
There was a problem hiding this comment.
don't use try/get, either expect the value or not.
| var line2 = rawAddress.Groups["line2"].Value.Trim(); | ||
| var property = string.Join( | ||
| ", ", | ||
| new[] { line1, line2 }.Where(value => !string.IsNullOrWhiteSpace(value)) |
There was a problem hiding this comment.
probably don't need to join line1 and line 2, just set the to address property and street.
|
|
||
| binDays.Add(nextBinDay); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(previousDateText)) |
There was a problem hiding this comment.
Could simplify with loop or something instead.
1 participant
Summary
This PR adds a new bin collection data collector for London Borough of Newham.
ICollectorinterfaceCloses #128
Test Summary
Generated automatically by Moley-Bot using Codex CLI