Skip to content

Gogogate2 0.1.1#14294

Merged
balloob merged 9 commits intohome-assistant:devfrom
dlbroadfoot:gogogate2-0.0.9
May 8, 2018
Merged

Gogogate2 0.1.1#14294
balloob merged 9 commits intohome-assistant:devfrom
dlbroadfoot:gogogate2-0.0.9

Conversation

@dlbroadfoot
Copy link
Copy Markdown
Contributor

@dlbroadfoot dlbroadfoot commented May 5, 2018

Description:

Update to use latest version of gogogate2 component. Expose device temperature as attribute

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:

Uses latest version of library which ensures commands to device are idempotent
def state_attributes(self):
"""Return the state attributes."""
data = super().state_attributes
data[ATTR_TEMPERATURE] = self._temperature
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined name 'ATTR_TEMPERATURE'

def supported_features(self):
"""Flag supported features."""
return SUPPORT_OPEN | SUPPORT_CLOSE

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

blank line contains whitespace

@syssi
Copy link
Copy Markdown
Member

syssi commented May 5, 2018

Please fix the build error!

balloob
balloob previously requested changes May 5, 2018
def state_attributes(self):
"""Return the state attributes."""
data = super().state_attributes
data[ATTR_TEMPERATURE] = self._temperature
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.

This should not be added as an attribute but instead be made into it's own sensor. This means that you will need to greate a gogogate2 component to handle the connection and then setup the cover and sensor platforms.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. For now, I'll update the PR to get the bug fix in and look at adding the component/sensors later.

For additional data points such as battery_level of the garage tilt sensor - should that be added as an attribute of the cover platform or should it be its own sensor too?

Removed temperature attribute until it can be re-implemented as a separate sensor.
@fabaff fabaff dismissed balloob’s stale review May 8, 2018 11:58

Comment addressed

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Thanks 🐦

ip_address = config.get(CONF_IP_ADDRESS)
name = config.get(CONF_NAME)
password = config.get(CONF_PASSWORD)
username = config.get(CONF_USERNAME)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

password = config.get(CONF_PASSWORD)
ip_address = config.get(CONF_IP_ADDRESS)
name = config.get(CONF_NAME)
password = config.get(CONF_PASSWORD)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

unexpected spaces around keyword / parameter equals

@balloob balloob merged commit eb551a6 into home-assistant:dev May 8, 2018
@balloob balloob mentioned this pull request May 28, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
* Gogogate2 - bump version

Uses latest version of library which ensures commands to device are idempotent

* Update requirements_all

* Expose sensor temperature

* update version

* import attribute

* Set temperature

* Remove temperature attribute

Removed temperature attribute until it can be re-implemented as a separate sensor.

* Update ordering

* Fix copy-&-paste issue
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants