Update PostNL to also contain distributions and letters #27551
Conversation
|
@iMicknl Feel free to comment. Let me know if you want to be added as codeowner and I will add you in this PR. |
MartinHjelmare
changed the title
|
Not sure why the tests are failing. I suppose they need to be restarted. |
|
@peternijssen CI for checking code styling/formatting fails. Please format your code using Black, by running this from the project root:
|
|
Thanks @frenck. It also ran now in the pre commit. I only noticed it's missing in the dev docs I believe: https://developers.home-assistant.io/docs/en/development_testing.html |
This is fine, I currently won't have many time to maintain it. :-) |
| add_entities([PostNLSensor(api, name)], True) | ||
| add_entities([PostNLDelivery(api, name)], True) | ||
| add_entities([PostNLDistribution(api, name)], True) | ||
| add_entities([PostNLLetter(api, name)], True) |
There was a problem hiding this comment.
The two new classes are more or less duplicating the existing code.
Remove PostNLDistribution and PostNLLetter but add a data object and a list of the available e sensors. E.g., openweathermap or bitcoin sensor.
There was a problem hiding this comment.
Not sure if I completely understand. The only thing that is common is a part of the init method, the properties and a part of the update method. I could create a data object, store the name etc in it, but that basically means that with the name property, I have to retrieve it from the data property and the creation still happens in the init of the current class, as it's not a common name.
Also the update method is different between both. Perhaps the shipment foreach loop could be in a data class which handles that, I am just wondering if that's enough gain for you.
Or am I thinking something different then what you ment?
|
Closing this as PostNL is currently broken. |
Breaking Change:
The PostNL component now also contains distribution of packages and letters, next to the already existing delivery of packages.
sensor.postnlhas now been renamed tosensor.postnl_delivery. Additionally,sensor.postnl_distributionandsensor.postnl_lettersbecame available. This component is now fully compatible with the PostNL Lovelace card.Description:
The component now also adds distributions and letters and can nicely work together with a custom Lovelace card.
I've marked the following issue as solved, as the original suggestion was to have a custom Lovelace card, which is now possible.
Related issue (if applicable): fixes #20696
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10749
Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest.requirements_all.txtby runningpython3 -m script.gen_requirements_all..coveragerc.