Skip to content

Add HomematicIP alarm control panel#15342

Merged
MartinHjelmare merged 6 commits intohome-assistant:devfrom
worm-ee:add-homematicip_cloud-alarm_panel
Jul 13, 2018
Merged

Add HomematicIP alarm control panel#15342
MartinHjelmare merged 6 commits intohome-assistant:devfrom
worm-ee:add-homematicip_cloud-alarm_panel

Conversation

@worm-ee
Copy link
Copy Markdown
Contributor

@worm-ee worm-ee commented Jul 7, 2018

Description:

Add support for HomematicIP security zone into alarm control panel

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

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 are only imported inside functions that use them (example).



async def async_setup_entry(hass, config_entry, async_add_devices):
"""Set up the HomematicIP climate from a config entry."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Stale docstring.

def state(self):
"""Return the state of the device."""
if (self._device.sabotage or self._device.motionDetected or
self._device.windowState == HMIP_OPEN):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do we know that the alarm was armed here, ie that we should trigger on motion or opened window?

@property
def device_state_attributes(self):
"""Return the state attributes of the alarm control device."""
return {}
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jul 8, 2018

Choose a reason for hiding this comment

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

If this is needed to not return the parent class device attributes, please add a comment about that here and return None instead.

@property
def device_state_attributes(self):
"""Return the state attributes of the alarm control device."""
return None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a code comment why we need to overwrite this property and return None. This would look weird not knowing about the parent class implementation.

@property
def device_state_attributes(self):
"""Return the state attributes of the alarm control device."""
"The base class is loading the battery proprety, but device doesnt"
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare Jul 9, 2018

Choose a reason for hiding this comment

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

Python comments are written like this, ie starting with a hash sign:

# This is a comment.

Copy link
Copy Markdown
Contributor Author

@worm-ee worm-ee Jul 9, 2018

Choose a reason for hiding this comment

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

Sorry... I was distracted.

@property
def device_state_attributes(self):
"""Return the state attributes of the alarm control device."""
# The base class is loading the battery proprety, but device doesnt
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

proprety -> property
doesnt -> doesn't

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

Can be merged when build passes.

@MartinHjelmare MartinHjelmare merged commit 4a6afc5 into home-assistant:dev Jul 13, 2018
@ghost ghost removed the in progress label Jul 13, 2018
awarecan pushed a commit to awarecan/home-assistant that referenced this pull request Jul 16, 2018
* Add HomematicIP security zone

* Update access point tests

* Fix state if not armed and coments

* Add comment for the empty state_attributes

* Fix comment

* Fix spelling
@balloob balloob mentioned this pull request Jul 20, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants