Property side-effects with @lt.on_set

[rwb27]

This adds basic side-effects to properties, along with some tests.

Methods may be decorated with @lt.on_set. The method will then be run whenever the property is set.

To-do:

• Add to public API docs
• Add to conceptual docs on data properties, and link from functional properties
• Test this against DataSetting as well as properties
• Add at least one example to a feature branch in the OFM codebase

This is intended to allow for basic validation or synchronisation, where the full power of a functional property is not needed. It runs before validation (if enabled), to allow coercion and to catch None in case someone's forgotten to return a value from on_set. This could become configurable in the future.

It may be desirable to also allow a similar decorator to add a validator to the Pydantic model, but that might require more thought, and is not for this PR.

Closes #237

[barecheck]

Barecheck - Code coverage report

Total: 97.04%

Your code coverage diff: 0.04% ▴

Uncovered files and lines

File	Lines
src/labthings_fastapi/properties.py	486, 504, 584, 1039, 1043, 1139, 1162, 1578

[rwb27]

From @julianstirling out-of-band:

• Simulation camera updates the canvas on certain propery changes
• Also when we have mapping thingslots we check if the key is available in the mapping when we change our selectors

[bprobert97]

Are any errors or warning messages raised if the function doesn't return a value in the on_set function?
Might be helpful to a user, and we can write a test for it

[rwb27]

Currently, I think there should be an error if an on_set function returns a value that's not valid against the property's model. That probably isn't very helpful in what it says, I should take a look.

I don't think it's possible to distinguish between not returning a value and returning None. If None is a valid value, I don't think there's a sensible runtime check that would work.

I will take a look at the validation error that gets raised and see whether it could have a helpful message added if there's an on_set function present.

[rwb27]

I have implemented (and tested) two checks:

1. I now test that the function has a return type annotation. I don't check it matches the property (that's complicated by deferred annotations) but it should mean mypy enforces that it has to return a value, if mypy is in use.
2. I check if the on_set function modified the value. If so, I check if the modified value is None. If that happens, I log a warning. I'm not 100% sure I like this - I would welcome thoughts.

I suppose I could ask the function to return (True, value) to make it easier to spot - but I'm not sure I like that either...

[bprobert97]

I like the checks that have been implemented. One other idea I had is to change what the function is expected to return, so that an implicit None becomes an immediate, catchable error (i.e. force all on-set functions to return a specific dataclass or something). Instead of asking users to make on_set return a raw value or None, we could require them to return a specific class.

Here is a kind of example:

from dataclasses import dataclass
from typing import Any

@dataclass
class SetValue:
    value: Any

# Inside the descriptor:
result = self.on_set_func(obj, value)

if result is None:
    raise ValueError(
        f"You forgot to return a value in {self.name}.on_set! "
        "You must return SetValue(new_value) or SetValue(None)."
    )

if not isinstance(result, SetValue):
    raise TypeError(f"on_set must return a SetValue instance, got {type(result)}.")

value = result.value

SetValue(None) means "I explicitly want to set this to None." An implicit None raises a loud, helpful error. The downside is a slightly more verbose API for the user.

Just an idea, maybe not a useful one! I think we keep the return type-hinting check regardless, I think thats super useful

[rwb27]

What about changing both the name and the mode of operation to look a bit like a Pydantic "wrap validator"?

class MyThing(lt.Thing):
    intprop: int = lt.property(default=0)

    @lt.wrap_setter("intprop")
    def _wrap_set_intprop(self, value: int, setter: lt.Setter[int]) -> None:
        # my logic goes here (exceptions prevent the value changing
        setter(value)
        # I can add more logic here if I want it to run after the value is set

lt.Setter[int] would be a callable accepting an int, and when it's called it would set the value. That way, it's clear which code executes before and after setting the value.

The documentation ought to make it clear that you must either call the setter (exactly once) or raise an exception. That way, it's clear when we have a logic error. I think the new name is likely less confusing, and removes any ambiguity around when it's called in relation to the value changing.

PS I've edited this comment a few times to remove stream-of-consciousness wittering. Sorry if you saw a previous version.

[rwb27]

Very helpful suggestion from @julianstirling: we should define (and implement, and test) what happens if an exception is raised by on_set.