Stricter ruff #180
Open
Stricter ruff #180
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
I've added a stub __set__ to BaseProperty to improve type checking. Other than that, no code changes.
The ActionDescriptor really confuses mypy. It would be nice to find a better way to reference actions.
This doesn't improve the fact that the generated class can't be type checked.
None of these should affect how the code actually runs. Return types of functions used as FastAPI endpoints are also specified as the `model` and thus I don't expect any change to the API.
This also combines the two runs of mypy on src and typing_tests, as they can now both use the stricter rules.
Using `str` was converting `pending` to `InvocationStatus.PENDING` which caused the websocket tests to fail. Using `.value` fixes the problem.
These weren't flagged by dmypy but did show up with mypy.
I added a __set__ to BaseProperty to satisfy mypy, so now there is a test to check it raises an error and is overridden.
Merged
Added the NotConnectedToServerError exception.
I test for an error condition, and also test using the endpoint functions not via HTTP. It was necessary to add a default value for Thing.path to make sure we raise the right error - this is needed by other checks that are introduced in #180.
FastAPI allows dependencies to be specified either with an annotated type, or by supplying Depends() as the default. The latter form can cause problems, e.g. by masking the fact that a dependency hasn't been injected. I've now changed all occurrences of `Depends()` to use annotated types instead, as required by the `FAST` ruleset in ruff.
Sphinx is configured with a Python script, but this is not part of the package, the examples, or the test code. I've therefore excluded it from the rules. I've also changed the exclusion to be specific to the docs config file, and not the whole docs folder, just in case we add scripts there in the future. This also means any example Python files will be checked.
I've added module docstrings and some full stops, so the docstrings in the example comply with the rules for the module source.
Return type annotations for __init__ are not overly useful, and are not required by `mypy`. However, other magic methods do need an annotation, and it's helpful to have `ruff` check this rather than waiting for `mypy`. I've annotated all remaining `__init__` methods so this linter rule now passes. I also fixed a missing import from the change to Annotated for FastAPI dependencies.
One assertion has simply been deleted (line 437) as it was not clear how it could ever fail. An explicit type check a couple of lines below ensures we will catch any errors here promptly, so robustness is not affected.
I've exempted these from coverage: they are both primarily for the benefit of mypy, as they only guard against weakrefs failing. The objects referenced won't be deleted until after the server shuts down, so these errors should never occur.
The action manager is added to a ThingServer during __init__ so it may never be None. The assertion is therefore not necessary.
I've excluded this from coverage: it's testing against the blocking portal being missing, in a dependency function. This dependency function is not intended to be called by users, and it is only evaluated by FastAPI while the server is running.
This was guarding a weakref to the Thing: this isn't an error we expect, or an error we should handle, so I've excluded the if block from testing. Coverage is unchanged: the assert statement was equivalent to the new block, it just didn't show as uncovered if the asertion passed.
This includes a couple of extra tests to ensure the right error is raised if the functions are called prematurely.
Eval is flagged as potentially insecure. This function shouldn't be run on user input, and already sanitises the string. `blob_type` is also in the process of being phased out in favour of subclassing Blob explicitly.
This will decrease coverage slightly.
Tests are needed to stop coverage decreasing, as otherwise the exception isn't raised, and shows as uncovered. These errors shouldn't occur in normal operation.
I've enabled a Ruff rule to disallow `except Exception:` and removed one instance of that from the codebase. I have left one `except Exception:` in test code (which captures exceptions in a thread for later analysis), and another in the MJPEG stream (which logs exceptions but does not crash the server if they happen in the MJPEG stream). The MJPEG code might be able to be eliminated now that streams stop more gracefully - but I don't want to mess with that until we're in a position to test it more extensively. I've swapped the `logging.error` for `logging.exception` so that the full stack trace is dumped to the log.
Test code that will fail without a path should mock one. I've added a path in 3 places where this was required in the test suite.
Exceptions raised when setting up things were being wrapped in an ExceptionGroup: this is fixed by using pytest.RasisesGroup, but that required updating pytest. The test that checked for errors when a non-Thing was added to the server was failing, because the config was invalid. I've fixed it, but we really ought to use a model here. I've also renamed the import to avoid confusion with `server`, so we now import the server module as `ts`.
This previously used an `assert` so that any types that weren't models were handled in the `except` block. I've changed this so that plain types are wrapped in a return statement after the except: block, which I think is clearer than the previous structure.
TestThing causes a warning in pytest: I've renamed to avoid the confusion.
I was looking for the best place to add a test, and noticed the comments/docstrings here needed improvement.
Previous tests didn't check what happened for bad action IDs or actions with no output. This is now tested.
This adds a proper unit test for model_to_dict that checks all code paths.
|
I've paid a little "coverage tax" so the coverage is now increased. I'm starting to contemplate allowing slight (<0.1%) coverage decreases, as the coverage rule is now making it hard to delete code... |
I think the check for decreased coverage is at times unhelpful because it can add to the temptation to just add tests that quickly touch any line of code rather than add clear specific tests. One thing we may want to consider is diff cover. This will check for missing coverage on new lines, so that it encourages specifically testing new code rather than adding any test to get the coverage up. |
julianstirling
requested changes
Aug 26, 2025
| @@ -67,10 +69,10 @@ def __init__( | |||
| self.kwargs = kwargs | |||
|
|
|||
| @overload | |||
| def __get__(self, obj: Literal[None], type=None) -> Self: ... # noqa: D105 | |||
| def __get__(self, obj: Literal[None], type: type[Thing] | None = None) -> Self: ... # noqa: D105 | |||
There was a problem hiding this comment.
This seems like a ruff bug? As it shouldn't want a docstring for an overload?
| # We can't use the decorator in the usual way, because we'd need to | ||
| # annotate the type of `body` with `self.model` which is only defined | ||
| # at runtime. | ||
| # The solution below is to manually add the annotation, before passing | ||
| # the function to the decorator. | ||
| if not self.readonly: | ||
|
|
||
| def set_property(body): # We'll annotate body later | ||
| def set_property(body: Any) -> None: # We'll annotate body later |
There was a problem hiding this comment.
This doesn't return None? I see that you say the body will be annotated later, I am not 100% sure if you mean this in terms of:
- Later I will come back and improve this line of code; or
- This function will be automatically type hinted later
Assuming it is the second one I think it is better to add a clearer doctring before the line and then rather than adding an incorrect -> None I would just #noqa it.
Comment on lines
+99
to
+112
| if v is not THING_CONTEXT_URL: | ||
| raise ValueError(f"{v} must be {THING_CONTEXT_URL}") # pragma: no cover | ||
| # excluded from coverage as this is hardcoded, so we shouldn't ever | ||
| # see the error. | ||
| else: | ||
| assert ( | ||
| if not ( | ||
| v[0] == THING_CONTEXT_URL | ||
| or v[1] == THING_CONTEXT_URL | ||
| and v[0] == THING_CONTEXT_URL_v1 | ||
| ) | ||
| ): | ||
| raise ValueError( | ||
| f"{v} must contain {THING_CONTEXT_URL}" | ||
| ) # pragma: no cover | ||
| # This is hard-coded, so is not an error we ever expect to see. |
There was a problem hiding this comment.
I don't understand what this is doing nor why it is excluded from coverage?
It seems very easy to test, just pass in the wrong values from a test and check that it really does error?
I think especially the chained OR and AND is something where I am not comfortable with the precedence of binary operators enough to be 100% sure what this line does.
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 adds a bunch of Ruff rulesets, notably the
Srules (which enforce no assertions) andFASTrules (which are FastAPI specific, and disallow the use ofDependsas a default value because it can cause confusing behaviour).There are quite a few minor code changes as a result of this, mostly swapping assertions for explicit exceptions. As that changes test coverage, a few tests are added to explicitly test for the error conditions. Swapping
assertforif...raisemeans that a test must be added to cover theraiseline, or coverage will decrease.I've also added
ANNwhich enforces type annotations. This is mostly done by the strictermypyrules, but it's nice having a Ruff check because it's much faster. Irritatingly, this complains about__init__being untyped: I feel like__init__does not need an annotation. However, I can't disable it for__init__without disabling other magic functions as well, so I've added explicit-> Noneannotations throughout the codebase.I added
BLEto disallowexcept Exception:blocks, and spotted that one had crept in with a pull request: this is removed.Changing asserts to exceptions also required a bunch of docstring updates, and proper mocking of
Thing.pathin some tests. I'd like to consolidate some of this code in a future PR.