-
Notifications
You must be signed in to change notification settings - Fork 2
Test decorated actions #177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Using a decorator on an action will result in a callable that doesn't have a signature, and instead has a __wrapped__ attribute. These tests check that doesn't cause problems with LabThings. test_locking_decorator.py is a more realistic test of decorators that anticipates their use to lock functions.
Hmm, this gave me 6 errors. But after reinstalling it now works. trying to revert the reported changes also still works. Trying to install from the |
That's very strange. when you say trying to revert the changes, you mean changes in your Python environment? I suspect it's not worth lots of investigation - perhaps we chalk it up to environment inconsistencies and move on, unless the issue comes back. At least now the issue should show up as a test failure if it does come back. |
Yes, sorry I was writing at the speed of thinking and had planned to go back to edit what I was saying, but my delayed flight suddenly started boarding! Only a few libraries changed, and I did pip freeze beforehand. I did manually revert the couple that seemed like semi-significant changes based on version number/being a more core library; but this made no difference. My I have tried 4 or 5 ways now to try to recreate the broken config and failed. So I am going to assume the problem is legacy and approve this. It is somewhat disconcerting not to know what changed, but hopefully having these tests will allow us to nail it down if it happens again. |
FWIW uv-managed environments don't have pip installed at all, so |
I've been playing with uv on windows for the first time today and I could see that happening on a windows machine as pip does just talk at system level. On linux UV if you run pip in a UV environment you get a message saying you are in an externally managed environment rather than trying to run at system level. So I am sure this isn't what happened. |
I wish Windows did that! |
In #101 and #156 it's clear there are situations where we'd like to be able to decorate actions. In #156 @julianstirling shared code that looked like it should work, but failed strangely.
This PR introduces test code for decorated actions. I was expecting it to fail, then I was going to explicitly support
__wrapped__
(which is howfunctools.wraps
works) and watch it turn green wearing a smug expression. However, the test passes already: It turns out that reading the docs and usinginspect.signature
rather than accessing magic attributes directly has paid off, because wrapped functions are already supported. Apparentlyfunctools.partial
objects should also work - this might be useful in a couple of places, but isn't relevant to this PR.Unfortunately this means I can't reproduce #156, but I think these are useful tests for a use case that I didn't know we supported.
This PR is based off the branch for #176 and should be merged afterwards
It only touches two test files, if more changes are showing, that's because the previous PR is not in yet. It may need a rebase after #176 is in.