-
Notifications
You must be signed in to change notification settings - Fork 10
Fix sqlite resource errors #482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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 addresses SQLite resource warnings by properly closing database connections throughout the codebase. The changes introduce context manager support for the Database class and update test fixtures and tests to ensure proper resource cleanup.
Key changes:
- Added
__enter__,__exit__, andclose()methods to Database class for context manager support - Updated test fixtures to use generators with proper cleanup (
db,db_seeded,test_db) - Modified CLI to use
ctx.with_resourcefor automatic database cleanup - Upgraded pytest (>=9.0.0) and mypy (>=1.19.0) dependencies
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Updated mypy and pytest version constraints to >=1.19.0 and >=9.0.0 respectively |
| pyproject.toml | Updated mypy and pytest versions; changed pytest configuration section name and added strict option |
| packages/climate-ref/tests/unit/test_database.py | Wrapped test using Database.from_config in context manager and added explicit close() calls |
| packages/climate-ref/tests/unit/executor/test_result_handling.py | Added filterwarnings decorators to suppress expected UserWarnings |
| packages/climate-ref/tests/unit/datasets/test_datasets.py | Changed test_db fixture from return to yield with cleanup |
| packages/climate-ref/tests/unit/datasets/test_cmip6.py | Fixed pandas warnings with proper DataFrame copying and deprecated infer_objects parameter usage; wrapped database in context manager |
| packages/climate-ref/src/climate_ref/datasets/base.py | Refactored load_catalog to use transform instead of deprecated apply |
| packages/climate-ref/src/climate_ref/database.py | Added context manager protocol and close() method for proper resource cleanup |
| packages/climate-ref/src/climate_ref/cli/init.py | Used ctx.with_resource to ensure database connection is closed when CLI exits |
| packages/climate-ref/conftest.py | Converted db and db_seeded fixtures to generators with proper cleanup; added database.close() in db_seeded_template |
| changelog/482.improvement.md | Added changelog entry documenting the cleanup of database connections |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Resolved warnings about open resources by cleaning up the db
Checklist
Please confirm that this pull request has done the following:
changelog/