WIP: Alternate implementation to only trigger on changes #79

dlashua · 2020-11-04T15:31:57Z

This is an alternate method of detecting changes to PR #76.

It provides changes, old_state, and new_state to func_args.

It uses state_trig_ident to determine what the trigger cares about and only fires the function if one of these has changed.

It only fires once per state_changed event, since State.update() is only called once per state_changed event.

It does NOT fire if domain.entity.attribute has changed when only domain.entity is included in state_trigger.

This PR could be extended to provide only_on_change=False kwarg functionality like #76 did.

This PR could be extended to provide any_attribute_change=True kwarg to allow any attribute change to fire the function.

This PR could also be extended to provide domain.entity.old.attribute pseudo-variables for checking in state_trigger.

craigbarratt · 2020-11-04T20:00:06Z

This looks much better - thanks for updating with the new approach.

While it's helpful to compute the changes and passing them to the trigger function, that will become unnecessary when value and old_value become magic objects, since the user can easily figure it out for themselves. I'd prefer not to grow the function arguments unless it's really compelling.

On the multi-trigger question, I think this strikes the right balance. The principle is this: each@state_trigger can only trigger at most once for a given state change event. So if a state value and multiple attributes are set together (so there is one state change event with multiple changes), then that should only cause each trigger to be evaluated once. Of course, one state change event could cause many different triggers to be checked and potentially fired. If instead a state variable value is set, and then right afterwards an attribute change is made, then there will be two state changed events, and that could cause two triggers if @state_trigger is true each time. Note that the first eval will have the new state value, but the old attribute, and the 2nd one will have both new values.

For "trigger on any change", how about this:

state variable "domain.entity" triggers only when that state value changes. This will be a breaking change, but I think users can easily adapt; I doubt many users depend on the old behavior.
attribute "domain.entity.attr" triggers only when that attribute value changes. This is new.
"domain.entity.*" triggers when any attribute value changes (not value). This is new.

The third form could also include the state value changes too. I could go either way, although I slightly prefer that it only means attribute changes, since if you want both value and attributes you can specify both in the same trigger. Thoughts?

I don't think we need to have any additional kwargs options, unless we are really concerned about the breaking changes for trigger only on change (which is actually a bug we are fixing), and "domain.entity" meaning just the state value changed.

I'm happy to accept this PR as a stepping stone, and I'll start working on the states as objects, which I now have some idea about how to approach. Then we can update after that.

BTW, the test fails because the trigger functions have new arguments which the tests aren't expecting. The output is very verbose, but there are particular lines that show which assert failed and the differences between the expected and actual value at that point.

dlashua · 2020-11-04T20:20:38Z

While it's helpful to compute the changes and passing them to the trigger function, that will become unnecessary when value and old_value become magic objects, since the user can easily figure it out for themselves. I'd prefer not to grow the function arguments unless it's really compelling.

I agree that growing the function arguments isn't ideal. Perhaps I can set some additional *.old variables and then cycle through them in trigger.py. I'll give that a shot.

I'll take a swing at implementing domain.entity.* as well as you described. And, while I'm not too opinionated on whether domain.entity.* should include the state as well, I can't imagine a situation where someone would want updates on all the attributes but not also the state. It seems like they'd either want to pick individual things or they'd just want EVERYTHING so they can sort it out themselves in code.

I'll dig through the test output again and see if I can correct it. Though, if I succeed at handling this change, it won't even be needed.

dlashua · 2020-11-04T22:11:38Z

@craigbarratt I've updated this branch to attempt this in a new way. My limited testing shows that it works just fine. However, I made a change to state.notify_var_get to ensure that the data put on the queue includes all new_vars. It seems okay, but I'm not sure why it wasn't this way to begin with, so maybe I've overlooked something and this is a bad thing to do.

craigbarratt · 2020-11-05T00:13:28Z

custom_components/pyscript/__init__.py

@@ -217,9 +217,16 @@ async def state_changed(event):
            "value": new_val,
            "old_value": old_val,


With f7169ac, "value" can be StateVar(event.data.get('new_state')), or None if new_state is not provided (eg, when state variable is deleted. Same for "old_value".

craigbarratt · 2020-11-05T00:14:57Z

custom_components/pyscript/__init__.py

+        if old_val is not None:
+            for attribute in event.data["old_state"].attributes:
+                new_vars[f"{var_name}.{attribute}.old"] = event.data["old_state"].attributes[attribute]
+


These lines shouldn't be necessary now, since the value and old_value contain the full states.

craigbarratt · 2020-11-05T00:18:22Z

custom_components/pyscript/trigger.py

-                        }
-                        if func_args['var_name'] in self.state_trig_ident:
+                    for var in self.state_trig_ident:
+                        if new_vars.get(var) != new_vars.get(f"{var}.old"):


You'll need to check if var is domain.entity (if so, just compare value with old_value) or domain.entity.attr(if so, compare getattr(value, attr) with getattr(old_value, attr)).

craigbarratt · 2020-11-05T00:54:38Z

custom_components/pyscript/state.py

@@ -115,12 +115,12 @@ async def update(cls, new_vars, func_args):
    @classmethod
    def notify_var_get(cls, var_names, new_vars):
        """Return the most recent value of a state variable change."""
-        notify_vars = {}
+        notify_vars = new_vars


This looks like a good simplification. One small change: it's probably better to do

notify_vars = new_vars.copy()

so that new_vars isn't modified in-place.

I honestly don't know why this wasn't working the way it was. I made this change just to be SURE it was what I expected it to be, and it fixed the whole thing. I guess cls.notify had something in it I didn't expect. But I'm not sure how.

craigbarratt · 2020-11-05T00:59:03Z

custom_components/pyscript/state.py

-            elif var_name in new_vars:
-                notify_vars[var_name] = new_vars[var_name]
+            # elif var_name in new_vars:
+            #     notify_vars[var_name] = new_vars[var_name]
            elif 1 <= var_name.count(".") <= 2 and not cls.exist(var_name):
                notify_vars[var_name] = None


This line is going to need some modifications. I still haven't fully though this through. The motivation is to avoid exceptions due to undefined variables (eg: undefined state variable or attribute) by setting all variables that are not found in cls.notify_var_last or new_vars to None, so they don't generate an exception when the expression is evaluated. The logic is bit trickier now: if, say, domain.entity.attr is in var_names, and domain.entity has been set to its StateVar, then there's no need to set domain.entity.attr to None since attr is now defined in StateVar.

Not sure if that makes sense. I'm happy to deal with this after merge.

craigbarratt · 2020-11-05T01:01:50Z

I made my "magic state object" commit f7169ac. Turned out to be very simple and elegant. Now you can do things like:

x = input_sensor.test1
x.last_updated

I added some comments to the PR about how to use these new objects. Hopefully they make sense. The idea is that value and old_value will now be StateVar objects, which look like a string but contain the state attributes too.

dlashua · 2020-11-05T01:31:40Z

Oooh. This new commit of yours will make this a lot easier. Excellent!

I'll rework this whole thing. And I can probably put state.notify_var_get back the way it was.

dlashua · 2020-11-05T13:34:18Z

replaced by #82

dlashua added 2 commits November 4, 2020 09:28

send old_state and new_state to func_args

1a5a7cd

process old/new state to determine changes

337889b

detect change without using func_args

4981144

dlashua added 2 commits November 4, 2020 16:26

changing test to see if it passes

8aa1230

nope, I don't understand tests.

8ceeeec

craigbarratt reviewed Nov 5, 2020

View reviewed changes

dlashua closed this Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Alternate implementation to only trigger on changes #79

WIP: Alternate implementation to only trigger on changes #79

dlashua commented Nov 4, 2020 •

edited

Loading

craigbarratt commented Nov 4, 2020 •

edited

Loading

dlashua commented Nov 4, 2020

dlashua commented Nov 4, 2020

craigbarratt Nov 5, 2020

craigbarratt Nov 5, 2020

craigbarratt Nov 5, 2020

craigbarratt Nov 5, 2020

dlashua Nov 5, 2020

craigbarratt Nov 5, 2020 •

edited

Loading

craigbarratt commented Nov 5, 2020

dlashua commented Nov 5, 2020

dlashua commented Nov 5, 2020

		@@ -217,9 +217,16 @@ async def state_changed(event):
		"value": new_val,
		"old_value": old_val,

WIP: Alternate implementation to only trigger on changes #79

WIP: Alternate implementation to only trigger on changes #79

Conversation

dlashua commented Nov 4, 2020 • edited Loading

craigbarratt commented Nov 4, 2020 • edited Loading

dlashua commented Nov 4, 2020

dlashua commented Nov 4, 2020

craigbarratt Nov 5, 2020

Choose a reason for hiding this comment

craigbarratt Nov 5, 2020

Choose a reason for hiding this comment

craigbarratt Nov 5, 2020

Choose a reason for hiding this comment

craigbarratt Nov 5, 2020

Choose a reason for hiding this comment

dlashua Nov 5, 2020

Choose a reason for hiding this comment

craigbarratt Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

craigbarratt commented Nov 5, 2020

dlashua commented Nov 5, 2020

dlashua commented Nov 5, 2020

dlashua commented Nov 4, 2020 •

edited

Loading

craigbarratt commented Nov 4, 2020 •

edited

Loading

craigbarratt Nov 5, 2020 •

edited

Loading