Add ty type checking and resolve errors - #112
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #112 +/- ##
==========================================
+ Coverage 98.50% 98.53% +0.02%
==========================================
Files 13 13
Lines 536 545 +9
==========================================
+ Hits 528 537 +9
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces changes aimed at making the existing type hints actually checkable (via Astral’s ty) and resolving type-checking errors by tightening backend typing, adjusting FastAPI startup handling, and updating tests accordingly.
Changes:
- Converted FastAPI startup logic to the newer
lifespanasync context manager and adjusted static file/resource paths. - Made
BaseBackendgeneric and updated backend implementations to provide concrete config types (cfg). - Updated tests to satisfy stricter typing and route filtering expectations.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
bibra/backend/base.py |
Makes the backend base class generic and introduces typed cfg. |
bibra/backend/dummy.py |
Updates dummy backend to use the generic base and initializes cfg. |
bibra/backend/greylitlm.py |
Types the backend config via BaseBackend[GreyLitLMConfig] and annotates cfg. |
bibra/backend/nuextract.py |
Types the backend config via BaseBackend[NuExtractConfig] and annotates cfg. |
bibra/backend/pdf_extractor.py |
Refines _chunk_score return type to match actual float scoring behavior. |
bibra/main.py |
Replaces @app.on_event("startup") with a lifespan handler; adjusts StaticFiles/FileResponse path handling. |
tests/test_api_routes.py |
Filters router routes to APIRoute instances to avoid non-API entries. |
tests/test_cli.py |
Adds importlib.metadata usage for version assertions. |
tests/test_greylitlm.py |
Adds ty ignores for PublicationMetadata(**metadata) in cases with alias keys. |
tests/test_main.py |
Updates startup failure testing to rely on lifespan via TestClient. |
tests/test_nuextract.py |
Adds ty ignores for PublicationMetadata(**metadata) in cases with alias keys. |
tests/test_pdf_extractor.py |
Adds assertions to narrow types (feats / scores not None) for stricter checking. |
Suppressed comments (2)
tests/test_greylitlm.py:241
- Using
ty: ignore[invalid-argument-type]here suppresses the type checker rather than constructingPublicationMetadatain a type-friendly way. SincePublicationMetadatais a Pydantic v2 model, prefermodel_validate()to parse the dict (including alias keys likee-isbn) without ignores.
expected = PublicationMetadata(**metadata) # ty: ignore[invalid-argument-type]
tests/test_nuextract.py:248
- Using
ty: ignore[invalid-argument-type]here suppresses the type checker rather than constructingPublicationMetadatain a type-friendly way. SincePublicationMetadatais a Pydantic v2 model, prefermodel_validate()to parse the dict (including alias keys likee-isbn) without ignores.
expected = PublicationMetadata(**metadata) # ty: ignore[invalid-argument-type]
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_greylitlm.py:188
- This test setup does not actually create a response object “without parts”, and assigning to str on a MagicMock instance won’t reliably affect str(mock_response) because special methods are resolved on the type. Use a simple object/class without a parts attribute and with a real str implementation instead.
mock_response = MagicMock()
del mock_response.parts
mock_response.__str__ = MagicMock(
return_value='{"language": "sv", "title": "Fallback Test"}'
)
Reasons for creating this PR
There are type hints in the code, but they are not necessarily correct as they are not validated.
Link to relevant issue(s), if any
Description of the changes in this PR
Set up ty to check type hints and resolve errors it found.
Also resolves the deprecation warning given by both ty and pytest, about FastAPI startup event.
Instructions how to test this PR
Run checks with
Known problems or uncertainties in this PR
Quite a few changes were needed because ty gave errors that I would categorise as false-positives. Is using type checking worth the extract effort its use requires?
Checklist
Disclosure of AI Tool Usage
Describe the AI tool(s) you used: