Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding thanet gov uk and wakefield gov uk #2954

Merged
merged 9 commits into from
Nov 2, 2024

Conversation

northerngeek
Copy link
Contributor

@northerngeek northerngeek commented Oct 31, 2024

Added Thanet District Council, UK in response to request #2928
Also adds Wakefield Council, UK in response to #305 and #1017 (both of these say there's a blocking issue, but I am not encountering it yet, not sure if the issue has disappeared?)

@northerngeek northerngeek changed the title Adding thanet gov uk Adding thanet gov uk and wakefield gov uk Oct 31, 2024
Comment on lines 65 to 66
url = f"https://www.thanet.gov.uk/wp-content/mu-plugins/collection-day/incl/mu-collection-day-calls.php?pAddress={self._uprn}"
collections_json = requests.get(url, headers= self.header_text).json()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to use the params parameter instad of encoding the arguments in the url string

        url = "https://www.thanet.gov.uk/wp-content/mu-plugins/collection-day/incl/mu-collection-day-calls.php"
        collections_json = requests.get(url, headers= self.header_text, params={"pAddress": self._uprn}).json()

return entries

def get_uprn(self) -> str:
url = f"https://www.thanet.gov.uk/wp-content/mu-plugins/collection-day/incl/mu-collection-day-calls.php?searchAddress={requests.utils.quote(self._postcode)}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the params argument instead of using requests.utils.quote

I think it's a good idea to check if self._postcode and self._street_address is set. If no argument is set (which is possible with this init arguments, the source throws TypeError: quote_from_bytes() expected bytes which is not really explanatory.

You could throw SourceArgumentRequired if postcode is missing and SourceArgumentRequiredWithSuggestions if postcode is set but not street_address

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've done this as described (a couple of days on from Hello World here so all of your points are really appreciated, I hope it's not too frustrating from your side. I've tried to learn by examples I have seen but these direct pointers are incredibly helpful).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not frustrating at all. I appreciate your work and am happy to help.

@5ila5
Copy link
Collaborator

5ila5 commented Oct 31, 2024

You do not need to create a new PR as you did with the Sefton PR. But you can just push changes to your northerngeek:adding_thanet_gov_uk branch.

@northerngeek
Copy link
Contributor Author

Thank you @5ila5, I hope I've done everything correctly, and thank you again for your help.

@5ila5 5ila5 merged commit 1125aaa into mampfes:master Nov 2, 2024
2 checks passed
@5ila5
Copy link
Collaborator

5ila5 commented Nov 2, 2024

Thanks for your contribution

@northerngeek northerngeek deleted the adding_thanet_gov_uk branch November 2, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants