Refactor MQTT scene to inherit MqttEntity #68883

jbouwh · 2022-03-30T07:03:24Z

Proposed change

Simplify MQTT scene by inheriting MqttEntity.
TODO: The documentation about the added enabled_by_default config option is to be corrected in next.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Mqtt scene add enabled by default home-assistant.io#22197

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

probot-home-assistant · 2022-03-30T07:03:31Z

Hey there @emontnemery, mind taking a look at this pull request as it has been labeled with an integration (mqtt) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

jbouwh · 2022-03-30T09:52:45Z

homeassistant/components/mqtt/scene.py

 PLATFORM_SCHEMA = mqtt.MQTT_BASE_PLATFORM_SCHEMA.extend(
    {
        vol.Required(CONF_COMMAND_TOPIC): mqtt.valid_publish_topic,
        vol.Optional(CONF_ICON): cv.icon,
        vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
-        vol.Optional(CONF_PAYLOAD_ON): cv.string,
+        vol.Optional(CONF_PAYLOAD_ON, default=DEFAULT_PAYLOAD_ON): cv.string,


This is in the documentation, but the default is was not set.

This is a breaking change, let's update the documentation according to the implementation instead.

already meregd

I'll open a PR to change the current docs that a default value is not set.

OK, let's revert this change then.

jbouwh · 2022-03-30T09:54:43Z

homeassistant/components/mqtt/mixins.py

@@ -811,7 +811,8 @@ async def _subscribe_topics(self):
    @property
    def entity_registry_enabled_default(self) -> bool:
        """Return if the entity should be enabled when first added to the entity registry."""
-        return self._config[CONF_ENABLED_BY_DEFAULT]


a scene cannot be disabled and will not have CONF_ENABLED_BY_DEFAULT as configuration option, despite if what the current documentation says. The PR to correct this is attached.

Why can't scene entities be disabled? Also, why do we change the common MQTTEntity?

my bad, CONF_ENABLED_BY_DEFAULT added to the schema. This option is removed from the current documentation, because it is not available in the current code.

emontnemery · 2022-03-30T11:27:19Z

homeassistant/components/mqtt/mixins.py

@@ -811,7 +811,8 @@ async def _subscribe_topics(self):
    @property
    def entity_registry_enabled_default(self) -> bool:
        """Return if the entity should be enabled when first added to the entity registry."""
-        return self._config[CONF_ENABLED_BY_DEFAULT]


Why can't scene entities be disabled? Also, why do we change the common MQTTEntity?

emontnemery · 2022-03-30T11:30:16Z

homeassistant/components/mqtt/scene.py

 PLATFORM_SCHEMA = mqtt.MQTT_BASE_PLATFORM_SCHEMA.extend(
    {
        vol.Required(CONF_COMMAND_TOPIC): mqtt.valid_publish_topic,
        vol.Optional(CONF_ICON): cv.icon,
        vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
-        vol.Optional(CONF_PAYLOAD_ON): cv.string,
+        vol.Optional(CONF_PAYLOAD_ON, default=DEFAULT_PAYLOAD_ON): cv.string,


This is a breaking change, let's update the documentation according to the implementation instead.

homeassistant/components/mqtt/scene.py

jbouwh · 2022-03-30T12:14:37Z

Correct current documentation: home-assistant/home-assistant.io#22196.
This PR removes the default value for payload_on.

emontnemery

Thanks, @jbouwh 👍

homeassistant/components/mqtt/scene.py

MQTT scene inherrits MqttEntity

425cc80

jbouwh requested a review from emontnemery as a code owner March 30, 2022 07:03

homeassistant added the cla-signed label Mar 30, 2022

probot-home-assistant bot added code-quality core integration: mqtt small-pr PRs with less than 30 lines. labels Mar 30, 2022

probot-home-assistant bot assigned emontnemery Mar 30, 2022

frenck added the smash Indicator this PR is close to finish for merging or closing label Mar 30, 2022

jbouwh force-pushed the mqtt-scene-inherrit-mqttentity branch from b29a545 to 425cc80 Compare March 30, 2022 08:49

jbouwh added 2 commits March 30, 2022 09:08

Default payload_on matches documentation

fc9d305

remove CONF_ENABLED_BY_DEFAULT from schema return True as default

ca6ef4d

jbouwh commented Mar 30, 2022

View reviewed changes

emontnemery reviewed Mar 30, 2022

View reviewed changes

emontnemery changed the title ~~MQTT scene inherrits MqttEntity~~ Refactor MQTT scene to inherit MqttEntity Mar 30, 2022

jbouwh added 2 commits March 30, 2022 11:52

Add to the schema, remove not overrided properties

0d88752

remove default payload_on

6146f2d

emontnemery approved these changes Mar 30, 2022

View reviewed changes

homeassistant/components/mqtt/scene.py Outdated Show resolved Hide resolved

Update homeassistant/components/mqtt/scene.py

e33ca9b

This was referenced Mar 30, 2022

Mqtt scene has no default for payload_on home-assistant/home-assistant.io#22196

Closed

Mqtt scene add enabled by default home-assistant/home-assistant.io#22197

Merged

emontnemery merged commit 4db2270 into home-assistant:dev Mar 30, 2022

jbouwh deleted the mqtt-scene-inherrit-mqttentity branch March 30, 2022 13:47

github-actions bot locked and limited conversation to collaborators Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MQTT scene to inherit MqttEntity #68883

Refactor MQTT scene to inherit MqttEntity #68883

jbouwh commented Mar 30, 2022 •

edited

probot-home-assistant bot commented Mar 30, 2022

jbouwh Mar 30, 2022

emontnemery Mar 30, 2022

jbouwh Mar 30, 2022

jbouwh Mar 30, 2022

emontnemery Mar 30, 2022

jbouwh Mar 30, 2022

emontnemery Mar 30, 2022 •

edited

jbouwh Mar 30, 2022

emontnemery Mar 30, 2022 •

edited

emontnemery Mar 30, 2022

jbouwh commented Mar 30, 2022

emontnemery left a comment

Refactor MQTT scene to inherit MqttEntity #68883

Refactor MQTT scene to inherit MqttEntity #68883

Conversation

jbouwh commented Mar 30, 2022 • edited

Proposed change

Type of change

Additional information

Checklist

probot-home-assistant bot commented Mar 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emontnemery Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbouwh commented Mar 30, 2022

emontnemery left a comment

Choose a reason for hiding this comment

jbouwh commented Mar 30, 2022 •

edited

emontnemery Mar 30, 2022 •

edited

emontnemery Mar 30, 2022 •

edited