labthings / labthings/labthings-fastapi
Matters arising from docstring review
Open
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 9
- Forks
- 4
- PR merge metrics
- No merged PRs in 30d
Description
This is a bit of a laundry list of things I noticed while doing the docstring review. They should be split into sub-issues in due course.
Conceptual pages needed
actions.__init__may want some content moving between module level docstring and a conceptual page. Actions are described in a couple of places but we need an overview of how the mechanism works.- In general, it would probably help developers to detail the code flow for the main activities. That's probably in its own developer-focused section of the docs, but I could see it really helping to describe, in some detail,
- How an Action is run, including the POST and subsequent GET requests, the resolving of dependencies, where the endpoint is defined, how the thread is started and monitored, and which functions are threaded vs async.
- How a Thing is defined, how its endpoints are generated, and how the Thing Description is made
- How properties work: get and set from Python vs HTTP
- MJPEG Stream: what code is threaded, what is async, and how we communicate between the two.
descriptorswill need to properly describe the lifecycle ofthing_settingand/or eliminate it in favour of always using the descriptor.- More detail of how and why to use dependencies other than the inter-thing dependencies.
- A description of how actions are cancelled, perhaps in the new actions page?
- A description of how the various dependencies work together to set up a new action - e.g.
InvocationID,CancelHook, ... - added to module docstring - A description of notifications/observers, including current status and planned improvements.
- Mention
fastapi_endpointsomewhere that talks about definingThings - Server configuration files.
- A page on documentation (Thing Description vs OpenAPI), I find there are many references to "TD and OpenAPI" anbd it would be nice to have a single target.
Code to tidy up or check
actions/__init__.py:377I've removedas_responsesas it should always be true - this makes type hints correct. I should makerequestnon-optional and update the 2 places where it's called.actions/__init__.py:191I tried typing this asActionDescriptorbut this cases confusion becausemypyseems to think the descriptor returned by the property will then be invoked with__get__. This is not correct. For now, I have removed the type annotation again to avoid the confusion. The resolution might be as simple as turning theactionproperty into a method, but we should consider this more carefully with some testing rather than have me bodge it now.descriptors/action.py:254should probably have aResponsedependency and pass it tolist_invocations.actions/invocation_model.py:47might be better typed asLogRecord?client- General: rename
tasktoinvocationto match naming conventions elsewhere. This is done in__init__and tests pass. - General: replace
ClientBlobOutputwithBloband get rid ofClientBlobOutput. client/__init__.py:234: should this haveBaseModelortype[BaseModel]? We're looking for the (sub)class, not an instance...client/__init__.py:24:_get_linkneeds error checking and might want to make use of the Model for links.client/in_server.pyneeds a fairly thorough rewrite. It is probably efficient to do this after client code generation is merged.
- General: rename
outputs/mjpeg_stream.py: review the locks and stream terminationtests/still usespoll_taskfromtemp_client.py. We should usepoll_invocationfromclientinstead (it's identical). We should also review howTestClientis used and perhaps make more use of the client module. This might want to wait until after code generation is implemented, as that will substantially change the client module.blocking_portalshould probably just be a property of.Thing.dependencies/blocking_portal.py:49I don't expect this exception to be raised. Is it worth a custom error? Or a test?- I've added
direct_thing_client_classtodepsand updated the dependencies example to use it. This is a change to recommended usage but not a change to the API beyond exposing another symbol. descriptors/action.py:198has an Exception-swallowing block. If this is needed, we should make it more specific or justify why not.descriptors/property.pywill be substantially rewritten. I have copied over docstrings from another branch that describe the status quo, I realise they are confusing, but that's why we plan to change the module significantly.example_things/__init__.pyshould be split up and renamed. Most of the things belong in tests, not in the module.outputs/blob/blob.pyshould use custom exceptions forretrieve_dataandto_dict. We also need more unit tests for blobs, including error conditions and invalid URLs.- 428: should use a custom exception, possibly based on
AttributeError. - 535: should use generic class methods to ensure the return type is an instance of
clsrather thanBlob. Same forfrom_bytes.
- 428: should use a custom exception, possibly based on
docs/src/blobs.rstcould really do with doctest to stop the example going staleoutputs/mjpeg_stream.pycould be simplified.buffer_for_readingis a pointless context manager, could be replaced withringbuffer_entry- many generic
RuntimeErrorsshould have exceptions defined. - Should we use
IndexErrorwhen frames aren't available (or at least a subclass thereof)? - Example code in the descriptor may want a doctest in due course.
- Could do with example code showing how it works in a simple camera?
server: could do with some more specific exceptions.__init__:80does this need to be a global? I think it might work without, and flake8 complains.
I've silenced it at the import and also the global line.
server.cli: need a model for config.thing_description:- Custom exception for
recursion_limit
- Custom exception for
utilities:LabThingsObjectDataprobably doesn't need to be a pydantic dataclass.- Do we want to centralise other key data in here, like
_settings_file_pathand_labthings_blocking_portal?
- Do we want to centralise other key data in here, like
introspection:- There's a confusing TODO about path parameters in
fastapi_dependency_params - There's a ValueError that might want subclassing in
input_model_from_signature.
- There's a confusing TODO about path parameters in
exceptionswill need to hoover up more exceptions. Do we define them here? probably yes...
notificationsis empty - need to consolidate code from property/action/websocket.thing:- consolidate settings into an object?
- default
thing_statedoes cacheing but this isn't really documented. Remove? - thing_description should consolidate
pathandbase_url. In fact, if we setbase_urlto be the path
to the TD, we can make everything else static.
- General: there are a lot of class attributes/annotations that should maybe be in
__init__. We need to pick a convention and stick to it, I have often defined class attrs next to the function(s) that use them, but it may be that defining instance variables in__init__is best practice? I've not seen an authoritative definition - whatever we do, it would be good to ensure it makes sense to e.g. VSCode and mypy.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
This is a multi-part review rather than a single task; begin by splitting the conceptual documentation requests and code checks into separate issues. For each sub-issue, start with the named files, line numbers, or tests, and define completion separately because this issue does not provide one bounded outcome.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- fastapi, openapi, python
- Domain
- backend-api-design, documentation, testing
- Issue type
- Documentation
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 15/100