Use type hints on properties instead of `model` argument by rwb27 · Pull Request #127 · labthings/labthings-fastapi

rwb27 · 2025-07-02T13:46:02Z

This branch builds on @julianstirling's type hint branch to eliminate the model argument from properties.

class ExampleThing(Thing):
    old_prop = ThingProperty(model=str | None, initial_value="Default)
    new_prop = ThingProperty[str | None](initial_value="Default")

    @thing_property
    def my_property(self) -> str | None:
        return "Default"

Both forms are now properly type hinted, tests with mypy to ensure inferred types are correct will happen in due course.

The tests are updated to use the new syntax, and ThingProperty will raise an exception if type hints are missing. Currently, specifying a model argument and no type hints will raise an error - this behaviour could be changed, but would result in less good type hints if it's allowed.

I've left the model argument as a double-check, if present it will be compared to the type hint and raise an error if it differs. We will probably remove it in due course.

barecheck · 2025-07-02T15:36:23Z

Barecheck - Code coverage report

Total: 92.34%

Your code coverage diff: 0.17% ▴

Uncovered files and lines

File	Lines
src/labthings_fastapi/descriptors/property.py	231, 253, 255, 296-297, 301, 425-426
src/labthings_fastapi/example_things/__init__.py	124

rwb27 · 2025-07-04T05:05:49Z

To be clear, I think this is mergeable but after v0.0.10. I propose:

we combine this and Adding typehints and overloads for typing properties #125
we change the signature of ThingProperty so initial_value becomes default and is the only positional arg.
we update tests and merge
refactor ThingProperty to split variable-like (with on_set) and getter-setter behaviour into separate subclasses

julianstirling · 2025-07-04T12:12:52Z

4. refactor `ThingProperty` to split variable-like (with `on_set`) and getter-setter behaviour into separate subclasses

Thinking about this a bit more whilst walking, the name on_set is slightly ambiguous whether it happens before or after setting. We could imagine that it could be used for validation (or even altering) the set value. Perhaps before_set() makes it clear that this happens before the value is set.

It may be useful to also have after_set() which could be used for example to perform an action once the value is set.

rwb27 · 2025-07-08T09:49:55Z

Thinking about this a bit more whilst walking, the name on_set is slightly ambiguous whether it happens before or after setting. We could imagine that it could be used for validation (or even altering) the set value. Perhaps before_set() makes it clear that this happens before the value is set.

It may be useful to also have after_set() which could be used for example to perform an action once the value is set.

I think there are cases for running code both before and after the value is set. I was envisaging that on_set would be run before the value is set, and that raising an exception in on_set would prevent the value from being set. Obviously that would need to be documented clearly - I am not wedded to the name if there's a better one. We could even have a decorator that's explicitly for validation if that's clearer, but maybe that's overcomplicating things.

In Javascript, the various on_whatever event handlers are run before taking the default action, and they can prevent the default action. My intended behaviour is in line with that, but of course javascript and Python are different languages. I'm not sure I know of a convention for this in Python.

I can't immediately see a use case for after_set, I guess it would mean that an exception raised in after_set doesn't prevent the value being updated, but still raises an exception in the code that sets it. Any scenario I can think of where the code goes wrong probably also makes sense to have the variable remain unchanged - but if there's a case for it, adding it does no harm.

rwb27 · 2025-07-08T09:53:28Z

Looking at my list of actions again, I would like to suggest we leave (2) (changing initial_value to default) for a future PR. That way, this is ready to merge, and I can focus on #138 until it's done. I'll rebase #138 if this gets merged in time.

In order to eliminate the `model` argument, we need to inspect the type annotation after __init__. This commit implements that behaviour, and adds some docstrings that were missing. In doing this, I think I really want to refactor and clarify the ThingProperty class - I will make an issue.

Now that ThingProperty is generic, we should make the decorator @thing_property generic. This should ensure properties have the type of the getter's return value.

This commit changes ThingProperty to raise an error if it is used without type hints. I have updated the test code accordingly, including tests for un-annotated properties raising an error. I've also tweaked the type hints on the @thing_property and @thing_setting decorators. MyPy was giving errors that their argument was a subtype of Thing rather than Thing, so I have now used `Callable[..., Value]` (i.e. the arguments are not checked, but the return value is).

Python < 3.12 wraps errors in __set_name__ in a RuntimeError. I've updated the test accordingly. We now test that either the exception is the expected one, or that the expected exception is the direct cause.

This was left behind when performing a rebase

julianstirling

I have left quite a few comments, mostly on doc strings where I was confused.

Having multiple ways to define the type seems very confusing. As do having so many modes of operation, especially modes that do not do what would be expected.

julianstirling · 2025-07-09T13:18:56Z

examples/opencv_camera_server.py

@@ -0,0 +1,292 @@
+import logging


This was added back in on the rabase! Sorry about this I will fix it.

julianstirling · 2025-07-09T13:19:22Z

examples/picamera2_camera_server.py

@@ -0,0 +1,227 @@
+from __future__ import annotations


This was also added back in on the rabase! Sorry about this I will fix it.

julianstirling · 2025-07-09T13:21:14Z

src/labthings_fastapi/descriptors/property.py



-class ThingProperty:
+class MissingTypeError(TypeError):


Are we assuming these are internal only or should we expose them in exceptions.py?

It's probably worth exposing. That could be in this PR or a future one - given that I'm planning to refactor it anyway it wouldn't be ridiculous to move the exception then?

julianstirling · 2025-07-09T13:28:18Z

src/labthings_fastapi/descriptors/property.py

+# There was an intention to make ThingProperty generic in 2 variables, one for
+# the value and one for the owner, but this was problematic.
+# For now, we'll stick to typing the owner as a Thing.
+# We may want to search-and-replace the Owner symbol, but I think it is
+# helpful for now. I don't think NewType would be appropriate here,
+# as it would probably raise errors when defining getter/setter methods.


I am suggesting updating this as I had to read it at least 10 times to understand what it meant

Suggested change

# There was an intention to make ThingProperty generic in 2 variables, one for

# the value and one for the owner, but this was problematic.

# For now, we'll stick to typing the owner as a Thing.

# We may want to search-and-replace the Owner symbol, but I think it is

# helpful for now. I don't think NewType would be appropriate here,

# as it would probably raise errors when defining getter/setter methods.

# Note: ThingProperty has a generic TypeVar for the value, but the `Owner` type

# is hard coded to `Thing` via a type alias.

# Making the `Owner` type generic has proved problematic. However, we have

# kept the `Owner` type to minimise code changes if this becomes more generic.

# Using `NewType` was considered, but not used as it will probably raise errors when

# defining getter/setter methods.

Fine by me - I won't commit it yet though, as it sounds like you might have more commits to make.

julianstirling · 2025-07-09T13:32:43Z

src/labthings_fastapi/descriptors/property.py

+        """
        self._name = name
+        value_types: dict[str, type[Value]] = {}
+        if hasattr(self, "_model_arg"):


Using hasattr rather than ensuring that the variable is set at _init_ is confusing. Setting the type to Optional, initialising with None, and checking is None is more explicit and clear.

There are a few places where None needs to be handled differently from an unset variable. I experimented with sentinel values but that seemed to be adding complication to something that could be solved more cleanly with hasattr.

If hasattr is considered bad I can probably reduce its use, though i will be hard to eliminate completely.

Is there a style guide that this comes from? If so I should probably read it...

julianstirling · 2025-07-09T13:58:51Z

src/labthings_fastapi/descriptors/property.py

        # Try to generate a DataSchema, so that we can raise an error that's easy to
        # link to the offending ThingProperty


I don't understand this comment. Is it a TODO? or is it explaining something else?

Ah, that comment referred to the line below (line 60 as-was): we ran type_to_dataschema(self.model) to ensure that any exception it raised would happen when the property was defined, and thus reference the right bit of code.

The function call and a similar comment should now be in __set_name__. This comment can be deleted.

Suggested change

# Try to generate a DataSchema, so that we can raise an error that's easy to

# link to the offending ThingProperty

julianstirling · 2025-07-09T13:59:56Z

src/labthings_fastapi/descriptors/property.py

+        if name in owner_annotations:
+            # If the property has a type annotation on the owning class, we can use that
+            value_types["class_annotation"] = owner_annotations[name]
+        if hasattr(self, "__orig_class__"):


Please define variable at init rather than using hasattr(self, ...

That is not possible. __orig_class__ is added by Python, after __init__ is called but before __set_name__. It's not a published language feature though, so I should probably add a link or two. While at least one of the posters warns it's not supported, it does appear to work in all the Python versions we support.

https://stackoverflow.com/questions/57706180/generict-base-class-how-to-get-type-of-t-from-within-instance

While it's technically an implementation detail, it has been available since Python 3.5 and the situations where it doesn't work don't apply to ThingProperty.

julianstirling · 2025-07-09T14:01:16Z

src/labthings_fastapi/descriptors/property.py

+            # We were instantiated as BaseThingProperty[ModelType] so can use that type
+            value_types["__orig_class__"] = typing.get_args(self.__orig_class__)[0]


I have no idea what this means!?

There are several ways to set the type. Rather than have a long and complicated nested if/else structure, I put all the specified types into the dictionary, then check all the values agree if there is more than one. I thought I'd added a comment to say so, but maybe it need improving...

julianstirling · 2025-07-09T14:02:45Z

src/labthings_fastapi/descriptors/property.py

+            )
+        print(
+            f"Initializing property '{name}' on '{owner}', {value_types}."
+        )  # TODO: Debug print statement, remove


Comments on the end of lines cause havoc with autoformatting because they unnecessarily wrap short lines making readability worse

eugh, yes that is nasty. I must have not checked that after running ruff format.

Though, in this case I should also just have deleted that line!

julianstirling · 2025-07-09T14:04:36Z

src/labthings_fastapi/descriptors/property.py

-        type_to_dataschema(self.model)

-    def __set_name__(self, owner, name: str):
+    def __set_name__(self, owner: type[Owner], name: str) -> None:


This function is doing so much I got to the end and had no idea what it had done. Does it need to be this complex? I think actually trying to write real unit tests that test every permutation of this function is going to be a close to impossible.

rwb27

I think there's a discussion to be had about whether this PR still makes sense, or whether it should be rolled up with a bigger change, that alters the signature of ThingProperty and gets rid of confusing behaviour.

If it is valuable to merge this in, as something that adds type hints but doesn't intentionally change existing behaviour (beyond raising errors if types are missisng), then it's worth revising and merging. However, after reading your comments I am starting to think it might make more sense to refactor ThingProperty as we've discussed elsewhere, and either alter this PR to do that, or close this one and start a new one - that might be altogether clearer.

I should also give thought to where I've used hasattr. I can probably eliminate some, though not all, uses of it.

rwb27 · 2025-07-09T14:18:56Z

src/labthings_fastapi/descriptors/property.py



-class ThingProperty:
+class MissingTypeError(TypeError):


It's probably worth exposing. That could be in this PR or a future one - given that I'm planning to refactor it anyway it wouldn't be ridiculous to move the exception then?

rwb27 · 2025-07-09T14:19:20Z

src/labthings_fastapi/descriptors/property.py

+# There was an intention to make ThingProperty generic in 2 variables, one for
+# the value and one for the owner, but this was problematic.
+# For now, we'll stick to typing the owner as a Thing.
+# We may want to search-and-replace the Owner symbol, but I think it is
+# helpful for now. I don't think NewType would be appropriate here,
+# as it would probably raise errors when defining getter/setter methods.


Fine by me - I won't commit it yet though, as it sounds like you might have more commits to make.

rwb27 · 2025-07-09T14:20:46Z

src/labthings_fastapi/descriptors/property.py

+    _model_arg: type[Value]
+    """The type of the model argument, if specified."""
+    _value_type: type[Value]
+    """The type of the value, may or may not be a Pydantic model."""


None is a valid type annotation - a perverse one, but it's valid. I settled on not setting the attribute when it has no value rather than using None because it eliminates some unnecessary type checking code.

rwb27 · 2025-07-09T14:26:37Z

src/labthings_fastapi/descriptors/property.py

+            behaves like a variable. The `getter` is only on first access.
+            The property may be written to locally, and whether it's writable
+            via HTTP depends on the `readonly` argument.
+        - If both a `getter` and `setter` are specified and `observable` is `False`,


hmm, yes that is ambiguous. I remembered incorrectly that a DirectThingClient could write to read-only properties that have a setter. Looking at the code, I see that the DirectThingClient property is made read-only. So readonly means that it's read/write on the Thing but read only from a ThingClient or DirectThingClient.

"locally" is the wrong term in that case - how about "may be written to when accessed as an attribute of the .Thing, but not by clients accessing it over HTTP or via a .DirectThingClient"?

rwb27 · 2025-07-09T14:27:19Z

src/labthings_fastapi/descriptors/property.py

+            documentation. Defaults to the first line of the docstring, or the name
+            of the property.


yes, defaults to the first line of the docstring if it's available, else the name of the property.

rwb27 · 2025-07-09T14:30:18Z

src/labthings_fastapi/descriptors/property.py

        # Try to generate a DataSchema, so that we can raise an error that's easy to
        # link to the offending ThingProperty


Ah, that comment referred to the line below (line 60 as-was): we ran type_to_dataschema(self.model) to ensure that any exception it raised would happen when the property was defined, and thus reference the right bit of code.

The function call and a similar comment should now be in __set_name__. This comment can be deleted.

Suggested change

# Try to generate a DataSchema, so that we can raise an error that's easy to

# link to the offending ThingProperty

rwb27 · 2025-07-09T14:33:38Z

src/labthings_fastapi/descriptors/property.py

+        """
        self._name = name
+        value_types: dict[str, type[Value]] = {}
+        if hasattr(self, "_model_arg"):


There are a few places where None needs to be handled differently from an unset variable. I experimented with sentinel values but that seemed to be adding complication to something that could be solved more cleanly with hasattr.

If hasattr is considered bad I can probably reduce its use, though i will be hard to eliminate completely.

Is there a style guide that this comes from? If so I should probably read it...

rwb27 · 2025-07-09T14:43:09Z

src/labthings_fastapi/descriptors/property.py

+        if name in owner_annotations:
+            # If the property has a type annotation on the owning class, we can use that
+            value_types["class_annotation"] = owner_annotations[name]
+        if hasattr(self, "__orig_class__"):


That is not possible. __orig_class__ is added by Python, after __init__ is called but before __set_name__. It's not a published language feature though, so I should probably add a link or two. While at least one of the posters warns it's not supported, it does appear to work in all the Python versions we support.

https://stackoverflow.com/questions/57706180/generict-base-class-how-to-get-type-of-t-from-within-instance

While it's technically an implementation detail, it has been available since Python 3.5 and the situations where it doesn't work don't apply to ThingProperty.

rwb27 · 2025-07-09T14:44:36Z

src/labthings_fastapi/descriptors/property.py

+            # We were instantiated as BaseThingProperty[ModelType] so can use that type
+            value_types["__orig_class__"] = typing.get_args(self.__orig_class__)[0]


There are several ways to set the type. Rather than have a long and complicated nested if/else structure, I put all the specified types into the dictionary, then check all the values agree if there is more than one. I thought I'd added a comment to say so, but maybe it need improving...

rwb27 · 2025-07-09T14:45:30Z

src/labthings_fastapi/descriptors/property.py

+            )
+        print(
+            f"Initializing property '{name}' on '{owner}', {value_types}."
+        )  # TODO: Debug print statement, remove


eugh, yes that is nasty. I must have not checked that after running ruff format.

Though, in this case I should also just have deleted that line!

rwb27 · 2025-07-09T15:29:02Z

On discussion with @julianstirling I think it is likely to be a poor use of time to bring this up to scratch. I am going to close it in favour of a more extensive refactor, which makes use of some of the code changes in here.

rwb27 marked this pull request as draft July 2, 2025 14:48

rwb27 changed the title ~~Use type hints on properties~~ Use type hints on properties instead of model argument Jul 2, 2025

rwb27 force-pushed the refactor_property branch from 6a6b462 to e7412fe Compare July 2, 2025 15:35

rwb27 marked this pull request as ready for review July 2, 2025 15:36

rwb27 requested a review from julianstirling July 4, 2025 04:57

rwb27 changed the base branch from typehint-property to main July 7, 2025 10:17

julianstirling and others added 8 commits July 8, 2025 15:30

Adding typehints and overloads for typing properties

0bb92b8

Make decorators generic

996f82c

Now that ThingProperty is generic, we should make the decorator @thing_property generic. This should ensure properties have the type of the getter's return value.

More detail on the "observable" property

a1f586b

Fix default argument name and missing type

31fbc24

Allow RuntimeError on older Python

ff05e2d

Python < 3.12 wraps errors in __set_name__ in a RuntimeError. I've updated the test accordingly. We now test that either the exception is the expected one, or that the expected exception is the direct cause.

Update CancellableCountingThing.check to new property syntax

2e684e7

This was left behind when performing a rebase

julianstirling force-pushed the refactor_property branch from e7412fe to 2e684e7 Compare July 8, 2025 14:46

rwb27 mentioned this pull request Jul 9, 2025

Docstring review #138

Merged

3 tasks

julianstirling reviewed Jul 9, 2025

View reviewed changes

rwb27 commented Jul 9, 2025

View reviewed changes

rwb27 closed this Jul 9, 2025

julianstirling mentioned this pull request Jul 9, 2025

Adding typehints and overloads for typing properties #125

Closed

		# Try to generate a DataSchema, so that we can raise an error that's easy to
		# link to the offending ThingProperty

		# We were instantiated as BaseThingProperty[ModelType] so can use that type
		value_types["__orig_class__"] = typing.get_args(self.__orig_class__)[0]

		documentation. Defaults to the first line of the docstring, or the name
		of the property.

Conversation

rwb27 commented Jul 2, 2025

Uh oh!

barecheck bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Barecheck - Code coverage report

Uh oh!

rwb27 commented Jul 4, 2025

Uh oh!

julianstirling commented Jul 4, 2025

Uh oh!

rwb27 commented Jul 8, 2025

Uh oh!

rwb27 commented Jul 8, 2025

Uh oh!

julianstirling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwb27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwb27 commented Jul 9, 2025

Uh oh!

Reviewers

Assignees

Labels

barecheck bot commented Jul 2, 2025 •

edited

Loading