Better event handling #256
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, BaseDescriptor typed the owning object as `Thing`, which was a bit loose and led to some vague type hints (particularly for functional properties and actions). I've added a second generic parameter for the owning class. This makes some of the type test code a bit more verbose, but it gets rid of a fair few `type: ignore` statements and also means we now detect a few type errors that were previously missed. This is mostly tidying up, but will be useful for some PRs in the near future.
This uses the new generic parameter for the descriptor's Owner to correctly type the function's `self` parameter, among other things.
This was a one-character fix: we now need the second argument when inspecting `__orig_class__` to get the value type. This is only used when a descriptor is created using subscript syntax, e.g. `prop = Property[Thing, int](default=0)`. Happily, it was picked up in tests, and is now passing.
This now correctly uses the `Owner` type variable in `BaseDescriptor`. `mypy` picked up an inconsistency between this and `ActionDescriptor`, but oddly not on Python 3.10.
This adds a new class that provides access to the useful methods of a BaseDescriptor, without needing to retrieve the BaseDescriptor object directly. This should tidy up code in a few places, where we want to refer to the affordances directly, not just their values.
This commit creates a new class, `BaseDescriptorInfo`. The intention is that using `BaseDescriptorInfo` will be more convenient than passing around descriptors. It may also be bound to an object, which should be significantly more convenient when both a Thing instance and a descriptor need to be referenced. An important side-effect that I'll note here is that `BaseDescriptor` is now a *Data Descriptor* as it implements a `__set__` method. This is arguably the way it should always have been, and simply means that `BaseDescriptor` instances won't get overwritten by the instance dictionary. Making `BaseDescriptor` instances read-only data descriptors by default means I can get rid of a dummy `__set__` method from `ThingSlot`.
This introduces the `DescriptorInfoCollection` class, and a descriptor to return it. The `DescriptorInfoCollection` is a mapping that returns `DescriptorInfo` objects. This makes it convenient to obtain both bound and unbound `DescriptorInfo` objects.
This includes tests for `PropertyInfo` and a fix to handle access to missing fields correctly. `.Thing.properties[<name>]` now returns a `PropertyInfo` object allowing access to the property's metadata. `.Thing.settings` does the same for settings, and also builds a model for loading/saving the settings. It does not, yet, load/save them, and needs test code.
This now means that a test for `isinstance(obj, self.value_type)` works as expected. I've added the descriptor name to a couple of error messages, for clarity, and improved docstrings to satisfy flake8.
This commit makes use of the new `Thing.settings` to generate a model for the settings file, and load/save the settings using a model. This has two advantages: * Settings that are typed as a model are now correctly loaded as a model. * Settings with constraints are validated when the settings file is loaded. The first point should get rid of some unnecessary code downstream. The second point is related to a change in behaviour: broken or invalid settings files, including those that have extra keys in them, now cause an error and will stop the server from loading.
This was accidentally defined on PropertyInfo, when it should have been in SettingInfo.
This is added, largely for completeness so it's consistent between actions, properties and settings.
This has rather run away with me and will need a lot of cutting-down. It attempts to: * Centralise event pub/sub handling in a `MessageBroker` * Align with the webthing subprotocol * Introduce models for more consistent and robust serialisation/deserialisation of websocket messages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has rather run away with me. It will need splitting and tidying before merging.
It started out attempting to centralise event handling into a
MessageBrokerclass, which would then mean events don't cause errors if they fire before the event loop starts. This basically worked.It also updated notification-related stuff to use the syntax
thing.properties[name].observe(stream)rather thanthing.observe_property(name, stream)which deduplicated the logic that found the descriptor.Lastly, and most problematically, it attempts to implement the
webthingsubprotocolsub-protocol for websocket messages. This requires a bunch of changes - in particular, the WoT standard has fewer statuses for actions ("cancelled" is gone).I should break it down so that we implement the first one, then the second one, then the third one - ideally in 3 separate PRs.