Add strict mypy checking #139
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds strict mypy type checking to the AstraDB library by upgrading mypy configuration and fixing type-related issues found during the stricter checking. The changes enable more rigorous type safety while maintaining code functionality.
- Upgrade mypy version and enable strict mode with additional error checking
- Fix type annotations and add runtime type validation
- Resolve import path issues and remove unnecessary type ignore comments
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Updates mypy version and enables strict type checking configuration |
langchain_astradb/cache.py |
Fixes type annotations for generators and improves sentinel object pattern |
langchain_astradb/utils/astradb.py |
Removes type ignore comments and refactors conditional assignment |
langchain_astradb/utils/vector_store_codecs.py |
Adds runtime type validation for document IDs |
tests/conftest.py |
Adds type validation for embedding input |
tests/unit_tests/test_callers.py |
Removes unnecessary type ignore comment |
tests/integration_tests/test_cache.py |
Fixes import path from relative to absolute |
tests/integration_tests/test_semantic_cache.py |
Fixes import path from relative to absolute |
tests/unit_tests/test_imports.py |
Adds test for the _reawaitable function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if self.result is _unset: | ||
| self.result = yield from self.awaitable.__await__() | ||
|
|
||
| yield |
There was a problem hiding this comment.
This bare yield statement will cause the generator to yield None before returning the result, which breaks the awaitable protocol. The __await__ method should only yield from the original awaitable when the result is not cached.
| yield | |
| return self.result |
There was a problem hiding this comment.
If we remove the yield __await__ is not a generator anymore.
| enable_error_code = "deprecated" | ||
| warn_unreachable = true | ||
|
|
||
| # TODO: activate for 'strict' checking |
There was a problem hiding this comment.
Is this left to false intentionally? I suppose that would be even stricter (not that I'm advocating it, just a matter of checking this todo)
There was a problem hiding this comment.
I see there are about 100 generics without type parameter (some are Astrapy objects, some are dicts and such).
If there is a plan to slowly improve towards compliance in a later PR (as I would empathize with), then the comment here could be perhaps made clearer in that regard?
(to be honest, astrapy has introduced type parameters for tables/collections not too long ago ...)
There was a problem hiding this comment.
Yes, that’s a todo for later.
|
|
||
| # TODO: activate for 'strict' checking | ||
| disallow_any_generics = false | ||
|
|
There was a problem hiding this comment.
Also while we're at it, there are 2 newlines here (on purpose?)
No description provided.