Added gogogate2 cover by dlbroadfoot · Pull Request #13467 · home-assistant/core

dlbroadfoot · 2018-03-26T10:13:28Z

Description:

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5023

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

cover:
  - platform: gogogate2
    username: email@email.com
    password: password
    ip_address: 192.168.1.200

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

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.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

homeassistant · 2018-03-26T10:13:31Z

Hi @dlbroadfoot,

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-03-26T10:13:33Z

+    try:
+        devices = mygogogate2.get_devices()
+        if devices == False:
+            raise ValueError("Username or Password is incorrect or no devices found")


line too long (85 > 79 characters)

houndci-bot · 2018-03-26T10:13:34Z

+
+    try:
+        devices = mygogogate2.get_devices()
+        if devices == False:


comparison to False should be 'if cond is False:' or 'if not cond:'

fabaff · 2018-04-01T09:34:27Z

+    vol.Required(CONF_USERNAME): cv.string,
+    vol.Required(CONF_PASSWORD): cv.string,
+    vol.Required(CONF_IP_ADDRESS): cv.string,
+    vol.Optional(CONF_API_KEY): cv.string,


Please add CONF_NAME to allow the user to overwrite the name of the device. Set the default to DEFAULT_NAME.

Added DEFAULT_NAME

fabaff · 2018-04-01T09:34:33Z

+                "Username or Password is incorrect or no devices found")
+
+        add_devices(MyGogogate2Device(mygogogate2, door) for door in devices)
+        return True


Simply return. It's not checked.

Removed True

fabaff · 2018-04-01T09:34:36Z

+            ''.format(ex),
+            title=NOTIFICATION_TITLE,
+            notification_id=NOTIFICATION_ID)
+        return False


Simply return. It's not checked.

Removed False

fabaff · 2018-04-01T09:34:38Z

+        self.mygogogate2 = mygogogate2
+        self.device_id = device['door']
+        self._name = device['name']
+        self._status = STATE_CLOSED


Let the device set the current state.

Set state from device

fabaff · 2018-04-01T09:34:41Z

+    @property
+    def should_poll(self):
+        """Poll for state."""
+        return True


This is the default.

Removed overridden property

fabaff · 2018-04-01T09:34:47Z

+
+    def update(self):
+        """Update status of cover."""
+        self._status = self.mygogogate2.get_status(self.device_id)


I guess that there will be an exception if the device is offline.

Caught exception and set state to STATE_UNAVAILABLE

houndci-bot · 2018-04-02T22:00:54Z

+            raise ValueError(
+                "Username or Password is incorrect or no devices found")
+
+        add_devices(MyGogogate2Device(mygogogate2, door, name) for door in devices)


line too long (83 > 79 characters)

dlbroadfoot · 2018-04-05T21:18:06Z

Hi @fabaff , I don't want to rush you but just wanted to check if you require me to do anything else before you want to review this again.

Thanks

houndci-bot · 2018-04-06T12:18:57Z

+            _LOGGER.error("%s", ex)
+            self._status = STATE_UNKNOWN
+            self.available = False
+


blank line at end of file
blank line contains whitespace

houndci-bot · 2018-04-06T12:18:57Z

+    def available(self):
+        """Could the device be accessed during the last update call."""
+        return self.available
+


blank line contains whitespace

fabaff

Thanks 🐦

MartinHjelmare

Commenting for future improvement.

MartinHjelmare · 2018-04-06T14:16:37Z

+
+        add_devices(MyGogogate2Device(
+            mygogogate2, door, name) for door in devices)
+        return


Not needed return.

MartinHjelmare · 2018-04-06T14:16:49Z

+            ''.format(ex),
+            title=NOTIFICATION_TITLE,
+            notification_id=NOTIFICATION_ID)
+        return


Same as above.

MartinHjelmare · 2018-04-06T14:17:11Z

+        self.device_id = device['door']
+        self._name = name or device['name']
+        self._status = device['status']
+        self.available = None


This should be made private.

@fabaff, @MartinHjelmare is correct. I just tried this out and this line currently throws an error 'can't set attribute'. I've changed it to private and will submit a new PR, along with the rest of Martin's suggestions.

New PR here: #13728

Thanks

MartinHjelmare · 2018-04-06T14:18:26Z

+    def close_cover(self, **kwargs):
+        """Issue close command to cover."""
+        self.mygogogate2.close_device(self.device_id)
+        self.schedule_update_ha_state(True)


Remove this. Since this is a polling entity, the state will be updated directly after the service call.

MartinHjelmare · 2018-04-06T14:18:46Z

+    def open_cover(self, **kwargs):
+        """Issue open command to cover."""
+        self.mygogogate2.open_device(self.device_id)
+        self.schedule_update_ha_state(True)


Same as above.

MartinHjelmare · 2018-04-06T14:19:44Z

+            self.available = True
+        except (TypeError, KeyError, NameError, ValueError) as ex:
+            _LOGGER.error("%s", ex)
+            self._status = STATE_UNKNOWN


Don't use STATE_UNKNOWN, just set the state to None.

Added gogogate2 cover

b1e0715

dlbroadfoot requested a review from andrey-git as a code owner March 26, 2018 10:13

homeassistant added the cla-needed label Mar 26, 2018

houndci-bot reviewed Mar 26, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Mar 26, 2018

dlbroadfoot mentioned this pull request Mar 26, 2018

Added documentation for gogogate2 home-assistant/home-assistant.io#5023

Merged

2 tasks

Hound fixes

9408408

homeassistant added the cla-signed label Mar 27, 2018

dlbroadfoot added 2 commits March 27, 2018 22:02

PR feedback

4f80a55

Hound comments

7c03e0a

fabaff reviewed Apr 1, 2018

View reviewed changes

houndci-bot reviewed Apr 2, 2018

View reviewed changes

dlbroadfoot added 2 commits April 4, 2018 07:44

Bump gogogate2 version

37c1500

Update requirements all

e52f2c0

home-assistant deleted a comment from houndci-bot Apr 6, 2018

Add device_class and features

a1c844a

homeassistant added the cla-signed label Apr 6, 2018

houndci-bot reviewed Apr 6, 2018

View reviewed changes

fabaff added 2 commits April 6, 2018 14:45

Fix lint issues

381a8d6

Again lint

403d3af

fabaff added 2 commits April 6, 2018 14:48

Fix imports

fa51dfd

Fix end of file

2cdf95d

fabaff approved these changes Apr 6, 2018

View reviewed changes

fabaff merged commit bd51143 into home-assistant:dev Apr 6, 2018

home-assistant deleted a comment from houndci-bot Apr 6, 2018

MartinHjelmare reviewed Apr 6, 2018

View reviewed changes

dlbroadfoot mentioned this pull request Apr 6, 2018

Fix Gogogate2 'available' attribute #13728

Merged

balloob mentioned this pull request Apr 13, 2018

0.67.0 #13856

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

Uh oh!

Conversation

dlbroadfoot commented Mar 26, 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 Mar 26, 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!

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!

dlbroadfoot commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabaff left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dlbroadfoot commented Mar 26, 2018 •

edited

Loading

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