Add basic get/set schedule actions #188
Conversation
This is primarily intended for demonstration/discussion purposes.
There was a problem hiding this comment.
My basic thought here is that, yeah, it'd be nice to present a higher-level schedule interface, but what you're proposing here is usable if it has good schema validation such that people can figure out what they're doing wrong. Your comment about "intended for advanced use" is a good hint that this is pretty low-level.
For a higher-level schedule interface, I wonder if using or subclassing Schedule would be useful & doable?
| schema={ | ||
| vol.Required("entity_id"): str, | ||
| # TODO: This could be a lot tighter... | ||
| vol.Required("schedule"): vol.Schema({}, extra=vol.ALLOW_EXTRA), |
There was a problem hiding this comment.
There are online JSON schema generators that will write a JSON schema given an example JSON body. They're not perfect but can generate a good starting point. Or a tool like Swagger can be used to create a JSON schema from scratch.
There was a problem hiding this comment.
Providing an endpoint on pykumo to get the schedule schema (and using it here) might be a good way to hide this detail away from hass-kumo.
There was a problem hiding this comment.
(see main comment about when we want to start enforcing the schema) Yeah, I tried a bit to get the schema set (partially just to learn about how Home Assistant handles these things). I ran into confusing issues with voluptuous, but can take another look if you'd like to see this.
There was a problem hiding this comment.
Even if it's not a full schema validation but just code (inside pykumo) checking that required stuff is present and in the expected format, with useful error messages returned if not, that would be something. OTOH that's the sort of thing schema validation is good at. But I haven't played with whatever validator HA is using.
So I guess my preference would be for a somewhat loosely-defined schema that validates just the minimum. I'm thinking just something to protect the code from crashing due to unanticipated values/formats. Later, if you're able to make a higher-level interface in future, it would be different endpoints and so the schema would be different.
Yeah, that's my thinking as well -- at least for a v1, I think it's fine to include it as an experimental feature. I think we may learn a bit more about what fields/values are accepted and how these are across devices, so not sure if we want to lock down the schema (yet). Let me know what you think.
Ooh, hadn't been following that -- very interesting. It looks like it could work, but it doesn't seem like it matches up exactly with our use cases. Some initial thoughts after playing around with it:
|
My hope is that a subclass of Schedule could provide additional UI fields and validation. But I have not looked at it at all. |
2 participants
This is primarily intended for demonstration/discussion purposes. Also my first Home Assistant component PR, so feedback highly welcomed! Related issue: dlarrick/pykumo#51
Is this the API we want? Get/set are pretty low level (which might be fine). With a little work, we could also make it easy to enable/disable a range of schedule slots. With more work, we could make it so that Home Assistant could set all the fields in a single slot.
Note that this doesn't work on its own -- it needs to be used in conjunction an updated version of PyKumo which includes dlarrick/pykumo#53.
Before finalizing:
manifest.json-- presumably once Schedule tweaks and bugfixes pykumo#53 there will be a new release to include that.