state_trigger only triggers on value changes (using StateVar)

[dlashua]

This works for me. Needs more testing.

Does not implement domain.entity.* functionality. Suggesting we save that for a future PR.

[dlashua]

OMG, I fixed a test AND caught a bug because of a test! GO TESTS!

[dlashua]

entity.domain.* is not valid python, so it's a bunch of extra hurdles to jump through to make this the way to designate "all attributes".

I suggest we either use something else -- entity.domain.all, perhaps -- or we indicate this functionality with a kwarg or we just don't implement it at all and require users to be specific about what they want to trigger on. Thoughts?

[craigbarratt]

This is look great.

The trigger on any change values (eg: "domain.entity" or "domain.entity.attr") aren't passed through the Python interpreter - they are separated and matched independent of the remaining expression. So that gives us flexibility in what syntax to use. I think it should just be a couple of lines of code to handle the .* case. When the code matches for plain entity or entity attribute (assuming the regexp allows the optional 3rd part to also match *), then if the 3rd part is "*" we simply check if any attribute has changed.

The drawback with .all is it is another case of potentially obscuring a real attribute; you wouldn't be able to trigger off any changes to an attribute called domain.entity.all. So I'm still in favor of using "domain.entity.*" to denote any attribute change.

[dlashua]

Excellent. I can see if I can figure out where to put that, but the deeper it gets into the Ast bits, the harder it is for me to decipher. Perhaps if you get some time you can figure out where to put it? Right now, if I put "domain.entity.*" into a state_trigger, I get an error like this:

2020-11-05 19:36:32 ERROR (MainThread) [custom_components.pyscript.file.bigtest.report] Exception in <file.bigtest.report @state_trigger()> line 1:
    binary_sensor.test.*
                       ^
SyntaxError: unexpected EOF while parsing (file.bigtest.report @state_trigger(), line 1)

[craigbarratt]

You need to update STATE_RE in trigger.py to match all three cases. Then that will cause all the "any change" triggers to get removed from the expression and saved in state_trig_ident_any. Then the trigger code needs to check state_trig_ident_any to see if any of the "any change" variables/attributes there have actually changed or not.

I guess STATE_RE isn't a very good name anymore - maybe STATE_ANY_CHANGE_RE?

[dlashua]

okay. I think I can manage that. I'll give it a shot now.

[dlashua]

@craigbarratt limited testing shows domain.entity.* feature to be working.

[dlashua]

@craigbarratt I think I covered everything. Let me know if I missed something.

[craigbarratt]

Is this whole if clause necessary? In this case ident_any_values_changed() would already have returned True.

[dlashua]

My thought process was that ident_any_values_changed() checked it, but the value did not change. Therefore, it didn't return True. So, in ident_values_changed() we can skip it (and save some CPU) since they were already checked. If we take the if out, since it didn't return True before, it won't return True now either.

[craigbarratt]

This is looking great - all the logic looks correct to me. I just added a few cosmetic comments.

I'm happy to merge after your next changes, and then I can add the same features to task.wait_until(). Or you are welcome to do that too. I think you'll need to make the two functions global so they can be called from both places.

[dlashua]

As yes, I can do the wait_until part as well. These functions will need to not only be global, but also take self.state_trig_ident_any, etc, as an additional parameter since it won't be there as "self".

[dlashua]

@craigbarratt limited testing shows task.wait_until() working as expected.

I don't use that feature much, so I wasn't sure what to test. I tested waiting for a state to change (and ensuring the wait didn't end because an attribute changed), wait for an attribute to change, waiting for "*", and waiting for an attribute to be a certain thing. They all seemed to work correctly.

[craigbarratt]

This looks good to me. I'm happy to merge if you don't have any other changes.

I'll add some tests after I merge.

[dlashua]

I think it's ready to go. Thanks!