Merged
Conversation
Add unit test, that is clear that docstrings should be updated if the behaviour changes
Barecheck - Code coverage reportTotal: 85.75%Your code coverage diff: 1.73% ▴ Uncovered files and lines |
Collaborator
Author
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
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 is replaced by #121
Currently, there are lots of imports from quite deep within this library that should be easier to find. @julianstirling suggested adding a useful set of top-level objects to the top level
__init__module, so thatcould become
It might even become a helpful convention to
import labthings as ltand then uselt.Thingetc.This feels like it should be simple, and all the symbols above work just fine. However, I have run into problems in the past when I tried to do this (specifically for
labthings_fastapi.dependencies) because of circular references. This might limit how many things can usefully be exposed at top level.Also, at the moment there is one
labthings_fastapimodule that is both client and server. Iflabthings_fastapiis installed without theserverextra, i.e. it's being used only as a client over HTTP, adding these top level imports will break it - becauselabthings_fastapi.thinghas a hard dependency onfastapi, for example. I suspect the resolution to that is to split the package into a client and server module - but I'd welcome opinions on what makes most sense.Depends on #110
N.B. this is currently based on @julianstirling's branch for #110 to avoid merge conflicts. It should not be merged before #110.