Skip to content

Add ty type checking and resolve errors - #112

Open
juhoinkinen wants to merge 8 commits into
mainfrom
issue48-type-checker
Open

Add ty type checking and resolve errors#112
juhoinkinen wants to merge 8 commits into
mainfrom
issue48-type-checker

Conversation

@juhoinkinen

@juhoinkinen juhoinkinen commented Aug 26, 2026

Copy link
Copy Markdown
Member

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.

  • Add ty as dev dependency, in precommit setup and as CICD check
  • Add instructions to AGENTS.md to run ty
  • Make type hints checkable and resolve type-checking errors

Also resolves the deprecation warning given by both ty and pytest, about FastAPI startup event.

Instructions how to test this PR

Run checks with

uv run ty check

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

  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

Disclosure of AI Tool Usage

  • 🟠 AI:ORANGE AI-generated at scale. Architecture and design actively co-developed and understood, hot spots reviewed, but not every line checked. Conscious risk of comprehension debt.

Describe the AI tool(s) you used:

  • Zoo Code with Qwen3.6-35B-A3B
  • Zoo Code with Claude Sonnet 4.6

@codecov-commenter

codecov-commenter commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.53%. Comparing base (ef35240) to head (50741d2).

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              
Flag Coverage Δ
unittests 98.53% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 lifespan async context manager and adjusted static file/resource paths.
  • Made BaseBackend generic 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 constructing PublicationMetadata in a type-friendly way. Since PublicationMetadata is a Pydantic v2 model, prefer model_validate() to parse the dict (including alias keys like e-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 constructing PublicationMetadata in a type-friendly way. Since PublicationMetadata is a Pydantic v2 model, prefer model_validate() to parse the dict (including alias keys like e-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.

Comment thread tests/test_greylitlm.py Outdated
Comment thread tests/test_nuextract.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"}'
        )

@juhoinkinen
juhoinkinen marked this pull request as ready for review August 27, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Type checker

3 participants