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

Mqtt light refactor #18227

Merged
merged 4 commits into from Nov 27, 2018

Conversation

@emontnemery
Copy link
Contributor

emontnemery commented Nov 5, 2018

Description:

Refactor of MQTT light as discussed in #18180 to add support for config entry to mqtt_json and mqtt_template

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

Example entry for configuration.yaml (if applicable):

Before change (mqtt_json):

light:
  - platform: mqtt_json
    command_topic: "home/rgb1/set"

After change (mqtt_json):

light:
  - platform: mqtt
    schema: json
    command_topic: "home/rgb1/set"

Before change (mqtt_template):

light:
  - platform: mqtt_template
    command_topic: "home/rgb1/set"
    command_on_template: "on"
    command_off_template: "off"

After change (mqtt_template):

light:
  - platform: mqtt
    schema: template
    command_topic: "home/rgb1/set"
    command_on_template: "on"
    command_off_template: "off"

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:

@emontnemery emontnemery force-pushed the emontnemery:mqtt_light_refactor branch 2 times, most recently from 95cb9ca to 55ba5e1 Nov 5, 2018

@OttoWinter
Copy link
Contributor

OttoWinter left a comment

This would be a breaking change. And especially for the MQTT discovered platforms I really do not like breaking changes as everyone would need to update their firmwares/wait for them to update too. It's not the end of the world, but maybe we could be a bit smarter with working around this?

I'm thinking: now that the platform option in discovery payloads has no use anymore, maybe we could use it as an alias for schema? This way we could have a gradual deprecation/removal cycle.

schema_basic.PLATFORM_SCHEMA_BASIC.schema).extend(
schema_json.PLATFORM_SCHEMA_JSON.schema).extend(
schema_template.PLATFORM_SCHEMA_TEMPLATE.schema).extend(
mqtt.MQTT_AVAILABILITY_SCHEMA.schema)

This comment has been minimized.

@OttoWinter

OttoWinter Nov 9, 2018

Contributor

This is not the way this should be done. If we already have a schema option, why not use it?

def validate_mqtt_light(value):
    SCHEMAS = {
        'basic': schema_basic.PLATFORM_SCHEMA_BASIC,
        'json': schema_json.PLATFORM_SCHEMA_JSON,
        ...
    }
    return SCHEMAS[value[CONF_SCHEMA]](value)

PLATFORM_SCHEMA = vol.All(vol.Schema({
    vol.Optional(CONF_SCHEMA, default='basic'): vol.All(vol.Lower, vol.Any('basic', 'json', 'template'))
}, allow_extra=True), validate_mqtt_light)

This comment has been minimized.

@emontnemery

emontnemery Nov 10, 2018

Contributor

Thanks!
However, what should the custom validator do if it finds a violation?
Print error messsage? Throw?

This comment has been minimized.

@OttoWinter

OttoWinter Nov 10, 2018

Contributor

Voluptuous already takes care of that using the code above. (Internally, the schema will throw an vol.Invalid or vol.MultipleInvalid which will propagate up through the custom validator into a validation error)

This comment has been minimized.

@OttoWinter

OttoWinter Nov 10, 2018

Contributor

Alternatively, we could do

PLATFORM_SCHEMA = vol.Any(schema_basic.PLATFORM_SCHEMA_BASIC, schema_json. ...)

# in basic schema
PLATFORM_SCHEMA = mqtt.MQTT_RW_PLATFORM_SCHEMA.extend({
  vol.Optional(CONF_SCHEMA, default='basic'): 'basic',
  # ...

# in other schemas (for example json)
PLATFORM_SCHEMA = mqtt.MQTT_RW_PLATFORM_SCHEMA.extend({
  vol.Required(CONF_SCHEMA): 'json',

but the validation error messages this alternative approach would produce are quite bad.

This comment has been minimized.

@emontnemery

emontnemery Nov 10, 2018

Contributor

Thanks a lot! Rewritten as you suggest.

Show resolved Hide resolved homeassistant/components/light/mqtt/__init__.py
Show resolved Hide resolved homeassistant/components/light/mqtt/__init__.py Outdated
Show resolved Hide resolved homeassistant/components/mqtt/discovery.py
Show resolved Hide resolved homeassistant/components/mqtt/discovery.py Outdated

@emontnemery emontnemery force-pushed the emontnemery:mqtt_light_refactor branch from 1a5bb43 to b39be91 Nov 10, 2018

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Nov 10, 2018

@OttoWinter Thanks a lot for reviewing! I added backwards compatibility by replacing platform in the discovery message with schema as you suggest.

@emontnemery

This comment has been minimized.

Copy link
Contributor

emontnemery commented Nov 21, 2018

How should the documentation be changed to reflect these changes?

Obviously, the configuration.yaml samples need to be updated and some wording rephrased (it's now one platform, supporting multiple schemas).
Should the documentation source files also be renamed in some way, or it does not matter?

emontnemery added some commits Nov 5, 2018

Add backwards compatibility for MQTT discovered MQTT lights.
Refactor according to review comments.

@emontnemery emontnemery force-pushed the emontnemery:mqtt_light_refactor branch from b39be91 to 4605e14 Nov 22, 2018

emontnemery added a commit to emontnemery/home-assistant.io that referenced this pull request Nov 22, 2018

@emontnemery emontnemery referenced this pull request Nov 22, 2018

Merged

Documentation for home-assistant/home-assistant#18227 #7601

2 of 2 tasks complete
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Nov 27, 2018

Docs should be renamed to just be light.mqtt. You can add redirect_from to make the old urls redirect to the new page.

@balloob balloob merged commit 16e3ff2 into home-assistant:dev Nov 27, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on mqtt_light_refactor at 93.032%
Details

@wafflebot wafflebot bot removed the in progress label Nov 27, 2018

balloob added a commit to home-assistant/home-assistant.io that referenced this pull request Nov 27, 2018

Documentation for home-assistant/home-assistant#18227 (#7601)
* Documentation for home-assistant/home-assistant#18227

* Merge documentation for the three shcemas

@emontnemery emontnemery deleted the emontnemery:mqtt_light_refactor branch Dec 2, 2018

debsahu added a commit to toblum/McLighting that referenced this pull request Dec 11, 2018

@balloob balloob referenced this pull request Dec 12, 2018

Merged

0.84 #19215

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment