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

Added gogogate2 cover #13467

Merged
merged 11 commits into from Apr 6, 2018

Conversation

Projects
None yet
5 participants
@dlbroadfoot
Copy link
Contributor

commented Mar 26, 2018

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:

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.

@dlbroadfoot dlbroadfoot requested a review from andrey-git as a code owner Mar 26, 2018

@homeassistant

This comment has been minimized.

Copy link

commented Mar 26, 2018

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!

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 26, 2018

line too long (85 > 79 characters)


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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Mar 26, 2018

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

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

@dlbroadfoot dlbroadfoot referenced this pull request Mar 26, 2018

Merged

Added documentation for gogogate2 #5023

2 of 2 tasks complete

dlbroadfoot added some commits Mar 27, 2018

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,

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

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

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Added DEFAULT_NAME

"Username or Password is incorrect or no devices found")

add_devices(MyGogogate2Device(mygogogate2, door) for door in devices)
return True

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

Simply return. It's not checked.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Removed True

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

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

Simply return. It's not checked.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Removed False

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

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

Let the device set the current state.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Set state from device

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

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

This is the default.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Removed overridden property


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

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 1, 2018

Member

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

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 2, 2018

Author Contributor

Caught exception and set state to STATE_UNAVAILABLE

raise ValueError(
"Username or Password is incorrect or no devices found")

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 2, 2018

line too long (83 > 79 characters)

dlbroadfoot added some commits Apr 3, 2018

@dlbroadfoot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2018

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

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

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

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

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 6, 2018

blank line at end of file
blank line contains whitespace

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

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 6, 2018

blank line contains whitespace

fabaff added some commits Apr 6, 2018

fabaff added some commits Apr 6, 2018

@fabaff

fabaff approved these changes Apr 6, 2018

Copy link
Member

left a comment

Thanks 🐦

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

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 94.16%
Details

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

@MartinHjelmare
Copy link
Member

left a comment

Commenting for future improvement.


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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

Not needed return.

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

Same as above.

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

This should be made private.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 6, 2018

Author Contributor

@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.

This comment has been minimized.

Copy link
@dlbroadfoot

dlbroadfoot Apr 6, 2018

Author Contributor

New PR here: #13728

Thanks

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

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

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

Same as above.

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

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Apr 6, 2018

Member

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

@balloob balloob referenced this pull request Apr 13, 2018

Merged

0.67.0 #13856

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.