Add PostNL sensor (Dutch Postal Services) by iMicknl · Pull Request #12366 · home-assistant/core

iMicknl · 2018-02-13T06:59:35Z

Description:

I have been working on a plugin which adds a PostNL sensor, which can be used for tracking package deliveries. It can be used to track multiple accounts, which is especially handy for households with several people.

Is there someone who can help me out with a code review / show me which parts I have to improve? I haven't touched Python before, so I think there are some parts I need to improve before merging. 👍

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io/pull/5231*

Example entry for `configuration.yaml` (if applicable):

- sensors
  - platform: postnl
    name: Mick
    username: [jouw.postnl.nl email]
    password: [jouw.postnl.nl password]

Checklist:

The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

homeassistant · 2018-02-13T06:59:36Z

Hi @iMicknl,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2018-02-13T06:59:45Z

+            return datetime.strptime(date.group(1).replace(' ', '')[:-6], '%Y-%m-%dT%H:%M:%S').strftime('%d-%m-%Y')
+
+        def parse_time(date):
+            return datetime.strptime(date.group(1).replace(' ', '')[:-6], '%Y-%m-%dT%H:%M:%S').strftime('%H:%M')


line too long (112 > 79 characters)

houndci-bot · 2018-02-13T06:59:45Z

+        status_counts = defaultdict(str)
+
+        def parse_date(date):
+            return datetime.strptime(date.group(1).replace(' ', '')[:-6], '%Y-%m-%dT%H:%M:%S').strftime('%d-%m-%Y')


line too long (115 > 79 characters)

houndci-bot · 2018-02-13T06:59:45Z

+        _LOGGER.exception('Wrong Credentials')
+        return False
+
+    add_devices([PostNLSensor(username, password, name, update_interval)], True)


line too long (80 > 79 characters)

houndci-bot · 2018-02-13T06:59:45Z

+    from postnl_api import PostNL_API
+
+    try:
+        api = PostNL_API(username, password)


local variable 'api' is assigned to but never used

thijsdejong

Not a code-reviewer or anything, just pointing out things I noticed that might need some improvement

thijsdejong · 2018-02-13T09:26:53Z

+        api = PostNL_API(username, password)
+
+    except Exception:
+        _LOGGER.exception('Wrong Credentials')


Might be better if this is changed to 'Error when logging in'. It currently will say Wrong Credentials if the PostNL site is down.

OttoWinter · 2018-02-13T10:12:09Z

+    @property
+    def name(self):
+        """Return the name of the sensor."""
+        return self._name or DOMAIN


Instead, consider using vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string, and DEFAULT_NAME = 'postnl' - it's cleaner to make voluptuous do default value handling.

OttoWinter · 2018-02-13T10:14:20Z

+        if self._state == 1:
+            return 'package'
+        else:
+            return 'packages'


pylint complains about a if-else-return here. Write this instead:

if self._state == 1: return 'package' return 'packages'

OttoWinter · 2018-02-13T10:16:17Z

+
+        api = PostNL_API(self._username, self._password)
+        shipments = api.get_relevant_shipments()
+        status_counts = defaultdict(str)


Why is this a defaultdict? I mean as far as I can see here, a normal built-in dictionary would work just fine...

OttoWinter · 2018-02-13T10:20:29Z

+        self._attributes = {
+            ATTR_ATTRIBUTION: 'Information provided by PostNL'
+        }
+        self._attributes.update(status_counts)


You could just write

self._attributes = { ATTR_ATTRIBUTION: 'Information provided by PostNL', **status_counts }

or alternatively, just remove status_counts and set the _attributes directly (then of course also removing one from self._state = len(status_counts)). Next, the attribution constant string "Information provided by PostNL" could also potentially be moved to a global constant (WUnderground, for example, does this - keeps the code a bit cleaner)

OttoWinter · 2018-02-13T10:22:38Z

+
+        for shipment in shipments:
+            status = shipment['status']['formatted']['short']
+            status = re.sub(r'{(?:Date|dateAbs):(.*?)}', parse_date, status)


Just a suggestion, but this seems like more of a thing the pip package should do instead of Home Assistant's code base...

I was in doubt about this one. The pip package is just a general wrapper for the PostNL API and I think this is something in the presentation layer, which shouldn't be in the pip package. However; maybe I should add some helper functions in the pip package in order to parse date / times.

fabaff · 2018-02-15T11:04:21Z

+        """Update device state."""
+        from postnl_api import PostNL_API
+
+        api = PostNL_API(self._username, self._password)


The setup of the API is already done in setup_platform. Pass that to PostNLSensor().

houndci-bot · 2018-02-25T15:51:42Z

+
+        def parse_time(date):
+            return datetime.strptime(date.group(1)
+                                     .replace(' ', '')[:-6], '%Y-%m-%dT%H:%M:%S').strftime('%H:%M')


line too long (99 > 79 characters)

houndci-bot · 2018-02-25T15:51:42Z

+
+        def parse_date(date):
+            return datetime.strptime(date.group(1)
+                                     .replace(' ', '')[:-6], '%Y-%m-%dT%H:%M:%S').strftime('%d-%m-%Y')


line too long (102 > 79 characters)

houndci-bot · 2018-02-25T15:51:42Z

+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/sensor.postnl/
+"""
+from collections import defaultdict


'collections.defaultdict' imported but unused

balloob · 2018-03-01T01:23:26Z

+    vol.Required(CONF_USERNAME): cv.string,
+    vol.Required(CONF_PASSWORD): cv.string,
+    vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
+    vol.Optional(CONF_UPDATE_INTERVAL, default=timedelta(seconds=1800)):


No need to implement this.

Implement the default as a constant SCAN_INTERVAL. Home Assistant will be able to pick that up and users can override it via their config: https://home-assistant.io/docs/configuration/platform_options/#scan-interval

balloob · 2018-03-01T01:24:03Z

+    try:
+        api = PostNL_API(username, password)
+
+    except Exception:


Specify the exact exceptions that you expect.

balloob · 2018-03-01T01:24:17Z

+
+    except Exception:
+        _LOGGER.exception("Can't connect to the PostNL webservice")
+        return False


Platforms don't return anything. Just return

balloob · 2018-03-01T01:25:13Z

+    def unit_of_measurement(self):
+        """Return the unit of measurement of this entity, if any."""
+
+        if self._state == 1:


unit should be static. Not being static breaks the history grouping. It sees it as different units. Just stick with packages.

balloob · 2018-03-01T01:27:08Z

+        shipments = self._api.get_relevant_shipments()
+        status_counts = {}
+
+        def parse_date(date):


homeassistant.util.dt has parsers for datetime, date and time in RFC3339 format.

balloob · 2018-03-01T01:28:50Z

+
+        for shipment in shipments:
+            status = shipment['status']['formatted']['short']
+            status = re.sub(r'{(?:Date|dateAbs):(.*?)}', parse_date, status)


Please include these parsing instructions in the postnl_api package and just return a parsed data structure to Home Assistant.

I will add is as a separate helper function in postnl_api, however I won't enable it by default since it tampers with the default output and it is also a small performance hit. However, I agree that this kind of parsing logic shouldn't be in the Home Assistant plugin itself.

houndci-bot · 2018-03-30T14:17:25Z

+import voluptuous as vol
+
+from homeassistant.components.sensor import PLATFORM_SCHEMA
+from homeassistant.const import (CONF_NAME, CONF_USERNAME, CONF_PASSWORD, CONF_SCAN_INTERVAL,


line too long (93 > 79 characters)

MartinHjelmare · 2018-03-31T10:37:38Z

+
+    def __init__(self, api, name, interval):
+        """Initialize the sensor."""
+        self.friendly_name = name


Remove this. This is for users to set.

MartinHjelmare · 2018-03-31T10:39:10Z

+    vol.Required(CONF_USERNAME): cv.string,
+    vol.Required(CONF_PASSWORD): cv.string,
+    vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
+    vol.Optional(CONF_SCAN_INTERVAL, default=SCAN_INTERVAL):


Remove this. It's already part of the config schema.

MartinHjelmare · 2018-03-31T10:39:19Z

+    username = config.get(CONF_USERNAME)
+    password = config.get(CONF_PASSWORD)
+    name = config.get(CONF_NAME)
+    update_interval = config.get(CONF_SCAN_INTERVAL)


Remove this.

MartinHjelmare · 2018-03-31T10:42:22Z

+
+        self._api = api
+
+        self.update = Throttle(interval)(self._update)


You can use the throttle if you want to limit the update interval customization. Create a constant for this and use that together with the throttle, eg MIN_TIME_BETWEEN_UPDATES = timedelta(seconds=60). The throttle will only be active per entity though so multiple entities can still hit the api faster than the throttle allows.

Please rename _update to update and put the throttle as a decorator on the update method.

Can you help me a little out with this one? put the throttle as a decorator on the update method.

Also, you said that SCAN_INTERVAL is already part of the config scheme. Why not use this for the throttling? So refer to SCAN_INTERVAL instead of creating a custom constant called MIN_TIME_BETWEEN_UPDATES.

@Throttle(MIN_TIME_BETWEEN_UPDATES) def update(self): ...

You don't want the throttling time to be configurable. You should decide on the min time between updates. Users will be able to customize the update interval down to that time.

Ok, thanks. Updated!
However, I am a bit lost how to implement the SCAN_INTERVAL and a default. The API can be queried every minute if you would like to, however I would suggest a default of every 15 minutes (or more) since this doesn't change that often. Any examples of other components who do something like this?

Just define SCAN_INTERVAL as a constant with the appropriate time interval at the module level. You can search for SCAN_INTERVAL to find examples.

MartinHjelmare · 2018-03-31T10:45:02Z

+import logging
+from datetime import timedelta, datetime
+import re
+import voluptuous as vol


Add a blank line between standard library and 3rd party imports.

houndci-bot · 2018-04-22T16:09:18Z

+        self._state = None
+
+        self._api = api
+        self.update = Throttle(config.get(CONF_SCAN_INTERVAL, DEFAULT_SCAN_INTERVAL))(self.update)


undefined name 'config'
line too long (98 > 79 characters)

iMicknl · 2018-04-22T18:51:03Z

@balloob / @MartinHjelmare thanks for the extensive reviews, both of you! I think I applied all the feedback and made the adjustments also in the Python wrapper + documentation.

Is it possible to review it for a last time, so I can finish it and ship it finally? :)

balloob · 2018-05-01T17:48:28Z

+
+ICON = 'mdi:package-variant-closed'
+
+MIN_TIME_BETWEEN_UPDATES = timedelta(seconds=120)


We should be less aggressive polling for new packages. Let's change this to 30 minutes.

True. In the beginning this was different, where the default was around 30. But where the user could change it himself. This changed after feedback..
Changed it to 30 minutes again.

USPS (American equivalent) has been IP banning people using the HASS integration…

# Conflicts: # homeassistant/components/sensor/postnl.py

balloob · 2018-05-02T17:45:44Z

Ok to merge when you run script/gen_requirements_all.py and commit the changes.

balloob · 2018-05-02T18:37:54Z

Congrats on getting your first PR merged 🎉

keesschollaart81 · 2018-05-02T18:39:10Z

Nice job @iMicknl

iMicknl · 2018-05-02T22:02:51Z

@balloob haha, it takes some time but finally. Thanks to all the reviewers! 👍 This was good for my Python skills.

iMicknl added 4 commits January 24, 2018 17:34

Add basic PostNL sensor (WIP)

8d719c1

Update PostNL sensor

3453a0e

Merge remote-tracking branch 'upstream/dev' into dev

4fd10de

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Bump version

f13e709

iMicknl requested a review from andrey-git as a code owner February 13, 2018 06:59

homeassistant added cla-needed new-platform platform: sensor.postnl labels Feb 13, 2018

houndci-bot reviewed Feb 13, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Feb 13, 2018

thijsdejong reviewed Feb 13, 2018

View reviewed changes

OttoWinter reviewed Feb 13, 2018

View reviewed changes

fabaff reviewed Feb 15, 2018

View reviewed changes

Small updates to PostNL package based on feedback

833237e

houndci-bot reviewed Feb 25, 2018

View reviewed changes

iMicknl added 3 commits February 25, 2018 16:52

Remove unused import

b2077c1

Pass api to sensor

f0d67e7

Refactor based on feedback

642075c

balloob reviewed Mar 1, 2018

View reviewed changes

Update based on feedback

d226104

houndci-bot reviewed Mar 30, 2018

View reviewed changes

MartinHjelmare reviewed Mar 31, 2018

View reviewed changes

Fix feedback

88882ca

iMicknl added 3 commits April 22, 2018 17:39

Bugfiix

8645ccc

Bugfix

1478716

SCAN_INTERVAL fix

409429d

houndci-bot reviewed Apr 22, 2018

View reviewed changes

iMicknl added 4 commits April 22, 2018 18:29

Remove unused import

85758ac

Refactor for new wrapper implementation

bce46c2

Update postnl package requirement

d68edb1

Change throttle logic

a88f9d3

iMicknl mentioned this pull request Apr 22, 2018

Add sensor.postnl documentation home-assistant/home-assistant.io#5231

Merged

2 tasks

iMicknl changed the title ~~WIP: Add PostNL sensor (Dutch Postal Services)~~ Add PostNL sensor (Dutch Postal Services) Apr 28, 2018

Update package version

b9cfd17

homeassistant added the cla-signed label Apr 28, 2018

houndci-bot reviewed Apr 28, 2018

View reviewed changes

Comment thread homeassistant/components/sensor/postnl.py Outdated

iMicknl and others added 2 commits April 28, 2018 21:02

Add new line

44f316a

Minor changes

c6523a6

balloob reviewed May 1, 2018

View reviewed changes

iMicknl added 2 commits May 1, 2018 20:03

Change refresh time to 30 minutes

9171873

Merge branch 'dev' of github.com:iMicknl/home-assistant into dev

9454e7a

# Conflicts: # homeassistant/components/sensor/postnl.py

balloob approved these changes May 2, 2018

View reviewed changes

Update requirements_all.txt

f213d09

balloob merged commit b66be59 into home-assistant:dev May 2, 2018

balloob mentioned this pull request May 11, 2018

0.69 #14392

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

ghost added integration: postnl and removed platform: sensor.postnl labels Mar 21, 2019


		self._api = api

		self.update = Throttle(interval)(self._update)


		ICON = 'mdi:package-variant-closed'

		MIN_TIME_BETWEEN_UPDATES = timedelta(seconds=120)

Uh oh!

Conversation

iMicknl commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thijsdejong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

iMicknl commented Feb 13, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

OttoWinter Feb 13, 2018 •

edited

Loading

OttoWinter Feb 13, 2018 •

edited

Loading

iMicknl May 1, 2018 •

edited

Loading