Add Nest away binary sensor and eta sensor#14406
Conversation
Comment out one line of code for security_state which need future version of python-nest
Comment out one line of code for security_state which need future version of python-nest
|
If there are 2 features, you should create 2 separate PR's EDIT: the PR is small enough to be reviewed together, so ignore this comment, take the comment in for future PR's only |
| # device specific | ||
| self.device = device | ||
| self._location = self.device.where | ||
| self._name = "{} {}".format(self.device.name_long, |
There was a problem hiding this comment.
from homeassistant.util import slugify
better then using custom format
There was a problem hiding this comment.
slugify is doing replace(" ", "_"), but I am doing replace("_", " ") here.
| else: | ||
| # structure only | ||
| self.device = structure | ||
| self._name = "{} {}".format(self.structure.name, |
There was a problem hiding this comment.
logic here is reverse version of slugify, but I will change "_" to single quote
|
@dgomes They are all from Nest's structure data, one is binary sensor, and another is sensor. Do I need to split them to separate PR? |
| @@ -190,8 +203,8 @@ def smoke_co_alarms(self): | |||
| for device in structure.smoke_co_alarms: | |||
| yield(structure, device) | |||
| @@ -204,8 +217,8 @@ def cameras(self): | |||
| for device in structure.cameras: | |||
| yield(structure, device) | |||
| if self.variable == 'operation_mode': | ||
| self._state = getattr(self.device, "mode") | ||
| elif self.variable == 'eta': | ||
| self._state = getattr(self.device, 'eta_begin') |
There was a problem hiding this comment.
I know that you are just following the existing code, but maybe these string should be moved to constants
at least harmonize the use of ' and "
There was a problem hiding this comment.
Yes, a dictionary should be used here:
SENSOR_VARS = {'operation_mode': 'mode',
'eta': 'eta_begin'}There was a problem hiding this comment.
I will change if/elif to if self.variable in VARIABLE_NAME_MAPPING:
|
@awarecan Please do not split this into two PRs. We are going to leave it as one single PR since it is already open, and because the two sensors rely on the same change(s) to the core component. |
|
This seems ready to merge. Does anyone else have any changes to request? |
|
LGTM |
5 participants
Description:
away and eta_begin are two data already in Nest structure data, but didn't exposed in HA for some reason. However, HA provide a service to change away mode, that is strange for me. So I want to expose those attribute to HA.
Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5352
Example entry for
configuration.yaml(if applicable):Auto discover/configuration sensors, no change on configuration
Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed: