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

Reconfigure MQTT lock component if discovery info is changed #19468

Merged

Conversation

Projects
None yet
3 participants
@emontnemery
Copy link
Contributor

commented Dec 19, 2018

Description:

Reconfigure MQTT lock component if discovery info is changed.
This PR is an extension of #18169 which introduced support for reconfiguring MQTT binary sensors.

(This is bullet 2 in home-assistant/architecture#70)

PRs for other platforms will be opened separately.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

@quazzie Can you help review this one?

@emontnemery emontnemery force-pushed the emontnemery:mqtt_discovery_update_lock branch from 0c902b3 to 9878969 Jan 11, 2019

@@ -180,8 +182,10 @@ def assumed_state(self):
This method is a coroutine.
"""
mqtt.async_publish(
self.hass, self._command_topic, self._payload_lock, self._qos,
self._retain)
self.hass, self._config.get(CONF_COMMAND_TOPIC),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

Use dict[key] for required config keys and keys with default config schema values.

self.hass, self._command_topic, self._payload_lock, self._qos,
self._retain)
self.hass, self._config.get(CONF_COMMAND_TOPIC),
self._config.get(CONF_PAYLOAD_LOCK),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See above.

@@ -193,8 +197,10 @@ def assumed_state(self):
This method is a coroutine.
"""
mqtt.async_publish(
self.hass, self._command_topic, self._payload_unlock, self._qos,
self._retain)
self.hass, self._config.get(CONF_COMMAND_TOPIC),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See above.

self.hass, self._command_topic, self._payload_unlock, self._qos,
self._retain)
self.hass, self._config.get(CONF_COMMAND_TOPIC),
self._config.get(CONF_PAYLOAD_UNLOCK),

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See above.

@@ -157,7 +159,7 @@ def should_poll(self):
@property
def name(self):
"""Return the name of the lock."""
return self._name
return self._config.get(CONF_NAME)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.

self._optimistic = False

availability_topic = config.get(CONF_AVAILABILITY_TOPIC)
payload_available = config.get(CONF_PAYLOAD_AVAILABLE)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.


availability_topic = config.get(CONF_AVAILABILITY_TOPIC)
payload_available = config.get(CONF_PAYLOAD_AVAILABLE)
payload_not_available = config.get(CONF_PAYLOAD_NOT_AVAILABLE)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.

availability_topic = config.get(CONF_AVAILABILITY_TOPIC)
payload_available = config.get(CONF_PAYLOAD_AVAILABLE)
payload_not_available = config.get(CONF_PAYLOAD_NOT_AVAILABLE)
qos = config.get(CONF_QOS)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.

payload)
if payload == self._payload_lock:
if payload == self._config.get(CONF_PAYLOAD_LOCK):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.

self._state = True
elif payload == self._payload_unlock:
elif payload == self._config.get(CONF_PAYLOAD_UNLOCK):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jan 13, 2019

Member

See below.

@emontnemery

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

Thanks a lot for reviewing!

Use dict[key] for required config keys and keys with default config schema values.

But why? Isn't it just error prone if dict[key] is accidentally used instead of dict.get() for optional keys without defaults?
On the other hand, if there's some good reason, should the other MQTT platform be updated too?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

We should always use appropriate tools for each task. If we think that the key is in the dict and use dict[key] and we're mistaken, we would get a KeyError and know about our mistake. That's good!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jan 13, 2019

Please update the other platforms in separate PRs.

@emontnemery emontnemery merged commit 0f3b6f1 into home-assistant:dev Jan 14, 2019

4 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

@ghost ghost removed the Ready for review label Jan 14, 2019

@balloob balloob referenced this pull request Jan 23, 2019

Merged

0.86.0 #20354

@emontnemery emontnemery deleted the emontnemery:mqtt_discovery_update_lock branch Jan 27, 2019

alandtse added a commit to alandtse/home-assistant that referenced this pull request Feb 12, 2019

Reconfigure MQTT lock component if discovery info is changed (home-as…
…sistant#19468)

* Reconfigure MQTT lock component if discovery info is changed

* Use dict[key] for required config keys and keys with default config schema values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.