Skip to content

Conversation

rwb27
Copy link
Collaborator

@rwb27 rwb27 commented Aug 13, 2025

I've added some mypy flags to enforce everything is annotated, and updated the codebase to get it passing the new, stricter checks.

mypy may now be run with mypy or dmypy run.

rwb27 added 11 commits August 15, 2025 00:23
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.
@rwb27 rwb27 mentioned this pull request Aug 16, 2025
Copy link
Contributor

@julianstirling julianstirling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realise that even though I left comments GitHub didn't mark it at review. I think the big one is the assert that should raise an actual error with a message.

@rwb27
Copy link
Collaborator Author

rwb27 commented Aug 25, 2025

I think this is ready - there are more improvements based on this branch in #180.

@rwb27 rwb27 requested review from julianstirling and removed request for julianstirling August 25, 2025 11:57
@rwb27
Copy link
Collaborator Author

rwb27 commented Aug 25, 2025

The assert is removed in #180 (I thought I may as well bite the bullet and remove all of them).

Trying to quickly do it here, I'm getting into a rabbit hole of rebasing faff. @julianstirling I think it would be more efficient for me to reset this branch back to f6c8976 (all tests passed), then we can hit "merge" and move on to #180 (which passes, but needs coverage improving).

I promise the assert is already gone in #180 - eliminating it in this one is just eating time I could use to fix something else.

@rwb27
Copy link
Collaborator Author

rwb27 commented Aug 25, 2025

@julianstirling I've reset this to the last commit that passed CI, which still includes an assert but is, I think, mergeable. I suggest we approve this unless there's anything else problematic, as it will then mean #180 can be reviewed more easily (because #180 is based on this branch, so it's currently showing both sets of changes). It already fixes the assert statement.

@rwb27 rwb27 requested a review from julianstirling August 25, 2025 18:26
@julianstirling
Copy link
Contributor

Separating no assertions from the MyPy changes is very sensible. I'm happy for this to be bumped to #180

@rwb27 rwb27 merged commit b90162d into main Aug 25, 2025
27 checks passed
@rwb27 rwb27 deleted the stricter-mypy branch August 25, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants