Conversation
julianstirling
left a comment
There was a problem hiding this comment.
I've tried to go through it. I think it is going to be annoying to have some property resetable and others not so I would really prefer FunctionalProperties to be resettable as quote a few of our properties have side effects and are still functional.
This being said it isn't too clear to me how the extra methods for a descriptor are defined hence some of the questions that go deeper.
| ) | ||
|
|
||
|
|
||
| class FunctionalProperty(BaseProperty[Owner, Value], Generic[Owner, Value]): |
There was a problem hiding this comment.
It would be nice if FunctionalProperty could allow setting a reset method as:
@myprop.reset
def _reset_myprop(self):
"""Resetting my property."""
self._myprop = 1066or even
myprop.default = 1066There was a problem hiding this comment.
Actually would it be possible to pass an an argument to the decorator?
_my_property=1066
@lt.property(default=1066):
def my_property(self):
...I suppose could even then be:
_my_property=1066
@lt.property(default=_my_property):
def my_property(self):
...There was a problem hiding this comment.
Having a reset function for a functional property makes sense to me, and I've deliberately left it so that this should be quite simple to implement. I was trying to keep this PR, so I've not added that feature in here, though if I'm adding more commits to add a reset endpoint, I could also add resetting for functional properties.
That said, if you've already reviewed this, it would be not unreasonable to merge it, and then add reset for functional properties in a future one.
Having a default for functional properties feels slightly odd to me, though as I write it I can't fully explain why. I think it's because the default for a DataProperty is the value it starts at - the same isn't necessarily true for a FunctionalProperty.
I really need to add side-effects! But that is an entirely separate PR...
There was a problem hiding this comment.
Created #297. It would be good to get it in for the same release as a feature asymmetry for functional properties makes it hard to adopt the feature for a consistent UI.
There was a problem hiding this comment.
Actually would it be possible to pass an an argument to the decorator?
I think this would be nasty to implement. I'm not saying a hard no, but I would be strongly in favour of providing a different syntax. The problem is that lt.property(default=<whatever>) is the same signature we use to create a DataProperty descriptor, so it would then require us to add a __call__ method to DataProperty such that the same class can function either as a decorator or a descriptor. That bakes my brain quite badly.
I think it would be absolutely fine to allow:
class MyThing(lt.Thing):
@lt.property
def myprop(self) -> int:
return 42
myprop.default=42default_factory could be done the same way, or exposed as a decorator - with perhaps a little thought as to whether the default factory ought to have access to self or not. Perhaps we'd actually want to suggest something like @myprop.default_factory_method to be explicit about that.
Similarly, @myprop.resetter would be a reasonable decorator to allow a reset operation.
This works on DataProperty and raises a specific exception for FunctionalProperty. It is not yet exposed over HTTP, but is tested in Python.
This will fail if the default won't validate. We probably want a check for this when the `Thing` is defined, as otherwise we will get annoying and unclear errors after the server has started.
This custom exception now ends with `Error` as recommended by @julianstirling during code review.
This PR adds a
defaultattribute to properties, where it can be defined. Currently, this applies only toDataPropertyandDataSettingas we don't have a neat way to define a default for functional properties. The default may be accessed in two ways:thing.properties['name'].default(from Python)defaultkey in thePropertyAffordancein the Thing Description.I have also added a
resetmethod, which resets the property to that value, if it exists.If you attempt to reset or get the default of a
FunctionalPropertywe raise a newFeatureNotAvailableexception which I hope makes it clear.The last step is to add a way to reset properties over HTTP. I am minded to make that a separate PR, because:
I can see three sensible ways to expose resetting over HTTP:
POST http://my.server:5000/my_thing/my_property/resetDELETE http://my.server:5000/my_thing/my_propertyPOST http://my.server:5000/my_thing/reset_propertywith payloadmy_property.Of these, I think we discussed
DELETEbefore and nobody liked it. I think the first one is clearest if we ignore WoT standards. The action is fine, but feels ugly, and currently it involves a lot more HTTP requests than it really needs, as the action would need polling to completion to find out whether it was successful.I'd be happy to merge this in its present state (i.e. without extra endpoints to reset) and we can discuss what to do about resetting. If we get a consensus quickly, I have also written the code to do resets, but not yet tested it.
Closes #257