Docs tidy after dropping dependencies #195
Open
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
In order to improve the Thing lifecycle, we're going to move to having the server instantiate the Thing. This will allow us to inject arguments to `__init__` that simplify the set-up process. This commit changes `add_thing` to accept a class rather than an object, and adds `args` and `kwargs` parameters to enable values to be passed to `__init__`. This commit swaps the `path` for a `name` (in all known cases this simply removes the leading `/` from the string) in preparation for making a distinction between names and paths. This commit does not change `Thing` at all: that will come later. It also doesn't change the test code - that will touch many more files!
This should help catch code that called the old version of add_thing more easily. Also, we no longer want `/` in the thing names, so I've removed that tfrom the regex.
For now, DirectThingClient should locate Things by name. It will be removed in due course, once thing connections are implemented.
I've taken this opportunity to move nearly all of the server set-up into fixtures. It would be nice to deduplicate this code in the future, but I think it's already neater than it was. Some tests use things that are not connected to a server. These will require mocking in the future.
This updates the code that's included as separate python files. I will also do a search for `.add_thing` and update docstrings.
I can't believe there are only two mentions - but these are the only two I can find so far.
This now consistently defines the arguments and return type.
There were #noqa directives left over from when we were double-checking `D` codes with flake8 and ruff. Ruff works better, so the #noqa is no longer needed.
Each Thing is now provided with a ThingServerInterface which allows it to interact with the server. The ThingServerInterface is specific to each Thing, and is how the Thing obtains its name and path. This removes the need to pass around the BlockingPortal: the ThingServerInterface provides a method to call async code in the event loop on the server. I hope this is clearer. The ThingServerInterface takes care of making sure we're connected to a server: I've therefore been able to remove a lot of error-handling code and tests for NotConnectedToServerError. This will need to be tested for explicitly, on the ThingServerInterface.
This came to light during testing of the new ThingServerInterface.. There's no reason errors setting up the logger should be swallowed by a catch-all exception: errors there are a bug in LabThings. Also, setting up the logger after emitting an event meant that an exception raised there caused another exception because of the missing logger. I have moved the creation of the logger out of the `try:` block to fix both these issues.
I now reference ThingServerInterface rather than BlockingPortal when describing how to run async code in the event loop.
We no longer need `get_blocking_portal` and can use `ThingServerInterface` instead. This change in `MJPEGStreamDescriptor` will require code that adds frames to delete an argument.
Some tests (specifically those that spin up an HTTP server) are much slower than the rest. I have marked these as "slow" so the test suite may be run with `-m "not slow"` when repeatedly re-testing. Obviously we should run all the tests in CI, this is a convenience feature for development.
This removes the blocking portal argument, that's no longer needed.
Now that we require a ThingServerInterface to create a Thing, I have provided a convenience test harness, `create_thing_without_server()` to consistently and cleanly allow `Thing` subclasses to be instantiated in test code. This auto-generates a name and path for the Thing, and mocks the required functionality. I have removed some tests that were checking for conditions where the Thing was not connected. Things are now created in a connected state, so this isn't needed any more. I should add tests for ThingServerInterface, particularly to make sure that it raises sensible errors when the event loop is not running. That's for a future commit.
This is a full, bottom-up set of tests for ThingServerInterface, as well as the associted mock/test harness code.
I've added a couple of ignores, for thing instantiation. Given that this is very much dynamically typed code, I don't think it loses anything.
This change makes it possible to load settings during __init__, though it does not yet do so. As the settings path is always available, we can remove some error-checking code that handled the period between the Thing being created and it having a settings path to use: this period no longer exists.
I had the same logic in `ThingServer.path_for_thing` and also `ThingServerInterface.path`. I've now called the former from the latter. I've then duplicated the logic again in `MockThingServerInterface` but I am less concerned about that, as duplication in a mock is not such a problem. There is a test to check the mock server interface generates a sensible path, and a test that asking for the path of a Thing that's not on the server raises an error.
I'm going to re-use the field-like type annotations for thing_connection, so it makes sense to move this into BaseDescriptor.
This implements a descriptor that will be used to connect Things together. It will need logic in the ThingServer before it is useful.
This commit adds the minimal set of features needed to use thing_connection in Thing code: * defines a function `thing_connection()` to make the class (primarily so we can type it as `Any` to avoid type clashes with field-style type annotation - see the related discussion for `property` in the typing notes section of the docstring). * adds a function to make thing connections in the server. * expose `thing_connection` in the API. * add a `name` property to `Thing` for convenience.
This checks that two things can be connected to each other. In writing this test, I realised my initial assumption that it would be easy to have circular dependencies and that these would not cause a problem wasn't true. See next commit for my solution to the circular dependency problem. The test covers the expected functionailty and anticipated errors in what I hope is a fairly bottom-up way.
The previous implementation of dataclass-style field typing (i.e. annotating class attributes like `foo: int = lt.property()`) used `typing.get_type_hints()`. This was clean and readable but wasn't very efficient (it retrieves every type hint every time) and un-string-ized the annotations immediately. I have switched to an implementation that is lower-level (using `inspect.get_annotations` and manually traversing the MRO) which has two key benefits: * We only evaluate the one type hint we care about each time. * The type hint is lazily evaluated as it's needed. The latter is important: it means that annotations given as strings or modules using `from __future__ import annotations` will work as expected. In implementing and type checking this I realised that it's far simpler to have both types of `Property` inherit all the typing behaviour (rather than trying to have `FunctionalProperty` inherit only part of it). This changes a few things, none of which I believe matter in code using the library: * Field-style type hints are now checked for functional properties. Previously, they were just ignored. Now, we'll check them and raise an error if a type is specified that disagrees with the supplied function This means errors will appear in some places where they'd have been ignored, but the behaviour for valid code is unchanged. * Types must always wait for the descriptor to be added to a class before they work. This mostly means tests that used bare `FunctionalProperty` instances had to be updated to put these in the context of a class (any class will do)..
`typing.get_type_hints` is the recommended way of getting the type hints for a class. Using this during `__set_name__` immediately evaluates any string annotations, i.e. it makes it impossible to do forward references. I'd previously rolled my own, using `__annotations__` and `eval` but this flagged security warnings with the linter, and misses some subtleties that are present in `get_type_hints`. While most of those subtleties aren't needed, I am prioritising code clarity over performance: instead of lazily evaluating the string type hint, I am just calling `get_type_hints` at a later point. This means there's one less bit of my code to go wrong.
It's now possible to specify the type as a Mapping or Union, in order to either permit multiple Things, or allow None.
There was duplicated validation code in ThingServer, which I've removed, in favour of using the ThingServerConfig model. I've also split up the code adding Things, so now we: 1. Create Things (and supply a ThingServerInterface) 2. Make connections between Things. 3. Add Things to the API. Step (3) may move out of __init__at some point in the future.
If the server is started from the command line, we print out any validation errors, then exit: a stack trace is not helpful. This also fixes an issue where ValidationError would not serialise properly, which stopped the test code working (it used multiprocessing).
I've removed ThingServer.add_thing in favour of passing a dictionary to ThingServer.__init__ and the test code is updated to reflect that. As a rule, when thing instances were required, I would: 1. Create the server 2. Assign `my_thing = server.things["my_thing"]` 3. Assert `assert isinstance(my_thing, MyThing)` That was enough for `pyright` (and I guess mypy) to correctly infer the type in my test code. It's slightly more verbose, but means we can use the same code in test and production, rather than needing a different way to create/add things in test code.
The name `thing_connection` confused people. `thing_slot` suggests we are marking the attribute and its value will be supplied later, which is exactly right. This commit makes that swap. I've searched/replaced every instance of ThingConnection and thing_connection: we should check the docs build OK, this is perhaps most easily done in CI.
This has been removed as its functionality is provided by `pydantic.ImportString`. It is no longer used for the fallback server: we gain nothing from the dynamic import. In the future, if we want to make it configurable, we could use `ImportString` there too.
Now that it's imported statically, `mypy` spots typing errors with the fallback server. These are now annotated correctly.
I've reworked core concepts and removed DirectThingClient, replacing it with a new "structure" page that I think preserves most of the still-useful content.
I've added an explanation comment with a link to the open pydantic issue.
:deco: now correctly formats cross-references as `@whatever` in the docs.
There was a bad indent in the properties module level docstring, and a few imports that confused Sphinx when making cross-references. I've moved an exception and tweaked the way Thing._thing_server_interface is declared to improve the docs: this should not change any code.
ThingServerInterface is used as a type hint for `Thing._thing_server_interface`. It was previously imported with an `if TYPE_CHECKING` guard, but as I've now explicitly type hinted and documented that attribute, any time `get_type_hints` is called on a `Thing` subclass (which is every time one is defined) it will cause an error. I've now imported it unconditionally, which fixes the problem. This does not cause any interdependency issues, as `thing_server_interface` has few onward dependencies.
Barecheck - Code coverage reportTotal: 94.39%Your code coverage diff: 0.74% ▴ Uncovered files and lines |
This shouldn't be duplicated in `conf.py`.
I've converted quite a few links to other pages in the docs, now that they exist. I think redirecting people to the WoT page all the time is causing confusion.
rwb27
force-pushed
the
docs-tidy-after-dropping-dependencies
branch
from
e915ef8 to
c9bfd2e
Compare
Contributor
There was a problem hiding this comment.
The file I want to comment on doesn't exist, but this is the same directory.
running_labthings.rst has a / in the path of mything on both lines 12 and 31
1 task
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 fixes a few issues with the documentation, mostly minor formatting stuff. It also adds a new docs tool that enables
:deco:to put the@in front of decorators.This follows on from #194 and should be reviewed after that one is merged - currently this looks like there are a lot more changes than there really are!