Fix typing for actions. #215
Merged
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
Collaborator
Author
|
The failures on unpinned deps are due to a change in FastAPI. This is a trivial fix but, for clarity, I'll do that in a separate PR. |
Collaborator
Author
|
@bprobert97 I thought you might like to see my typing tests expand (and, indeed, drive development). I think one of you and Julian is sufficient: I don't think this needs double review. |
bprobert97
reviewed
Dec 8, 2025
Contributor
|
Do the tests on GitHub run on the branch or the merged result? Probably best to fix the unpinned deps and then check this all passes? |
Contributor
|
Happy with code changes once tests pass |
Following the example of properties, I've added some test code that will make `mypy` flag issues with typing actions. This new test currently fails, showing that the current Descriptor is not typed correctly.
This introduces two changes: 1. actions are now properly type hinted. 2. ActionDescriptor now inherits from BaseDescriptor. The significance of these two changes will be described at more length in the MR thread. Inheriting from BaseDescriptor deduplicates some boilerplate, and deduplicates handling of name/title/description. The main change is that we now use TypeVar and ParamSpec to fully describe the function being wrapped. ActionDescriptor is generic in three parameters, which allows us to pass the type of the bound function through the descriptor properly. There was an assumption that the name of the function matches the name of the descriptor. This is now explicit. Currently, the only fudge is that the type of the owning Thing isn't perfect. This is a limitation of BaseDescriptor, which doesn't have a generic parameter for the owning Thing.
We were creating a new subclass of ActionDescriptor each time we instantiated it in `lt.action`. I'm fairly sure this was done because, in the past, we manipulated `__init__` or similar. That's no longer an issue. I have added a test to make sure rewriting `self.__doc__` has the intended effect.
Removed a TODO in favour of an issue, and expanded an acronym.
julianstirling
approved these changes
Dec 9, 2025
Contributor
julianstirling
left a comment
julianstirling
left a comment
There was a problem hiding this comment.
Thanks for the rebase, tests are now passing.
Collaborator
Author
|
I've rebased on #216 so all tests now pass. |
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.
Following the example of properties, I've added some test code that will make
mypyflag issues with typing actions.This new test currently fails, showing that the current Descriptor is not typed correctly. I will try to get this mergeable quickly, in the hope I can sneak it into v0.0.12 as a bug fix: I had hoped actions could be type checked already.
My intention is to:
ActionDescriptorinherit fromBaseDescriptor(to tidy up name/title/description handling and ensure__get__is consistent).ActionDescriptorgeneric, so that it correctly describes the bound function.lt.actioncorrectly, so that it can be type checked.