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

Add support fo "Renhållningen Kristianstad" in Kristianstad, Sweden #2741

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

hansbacklund42
Copy link

Added support for "Renhållningen Kristianstad" in Sweden to HACS waste collection service

Copy link
Collaborator

@5ila5 5ila5 left a comment

Choose a reason for hiding this comment

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

Why did you name the source krab_se? We typically name the source after the website ULR so renhallningen_kristianstad_se here.

Did you do any testing before submitting this? The source is fundamentally broken at multiple points. Please test your source using the test script (test_sources.py) before submitting a PR

Comment on lines 24 to 32
params = [{"query": self._street_address}]
headers = {"Module": "universal",
"Accept": "application/json, text/plain, */*",
"Unit": "dd905ce7-b16d-4422-be36-564169af4035"}
response = requests.get(
"https://api-universal.appbolaget.se/waste/addresses/search",
headers=headers,
params=urllib.parse.urlencode(params),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work you cannot urllib.parse.urlencode a list, You don't need to encode anything just make your params a dict and pass it as params the request's library does the converting for you (probably with the same method)

Comment on lines 42 to 50
params = [building_id]
headers = {"Module": "universal",
"Accept": "application/json, text/plain, */*",
"Unit": "dd905ce7-b16d-4422-be36-564169af4035"}
response = requests.get(
"https://api-universal.appbolaget.se/waste/addresses",
headers=headers,
params=urllib.parse.urlencode(params),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

building_id is not passed as a param at all but as part of the url:

response = requests.get(
    "https://api-universal.appbolaget.se/waste/addresses" +building_id,
    headers=headers
)

params=urllib.parse.urlencode(params),
)

data = json.loads(response.text)["services"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

its data = json.loads(response.text)["data"]["services"]

Comment on lines 39 to 40
if not building_id:
return []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please raise an Exception instead of just returning [], So There is an error message in the GUI while configuring or in the logs if this happens with an already configured source

@hansbacklund42
Copy link
Author

First of all naming conventions and naming.
I used that name because the company providing the service of collecting waste is KRAB and in Sweden.
It's an abreviation of "Kristianstad Renhållning AB" or as a weird translation it would be something like "Kristianstad Wastecollection Stock-Company" (or just Company, Ltd).
Depending on the definition of service provider the service provider it maybe should have been more like "api_universal_appbolaget_se" but that's just a url to a REST service on a consultant company and doesn't really define this waste collection.
I looked swiftly at some other implementations there are not a very strict naming convention as some refer to a more generic domain and some takes the name from a minicipality site not specific to waste collection.
But the name can of course be changed if that is critical.

I'm sorry for the code as it's really so many things that's wrong there. There's no other way of sayng it than it's totally FU.
It's a weird mix of what's running on my Home Assistant and some early mockup making it even worse than the mockup.
I'll be back with a working code.

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