Skip to content

Add types to _get_db#12462

Merged
mekarpeles merged 1 commit intointernetarchive:masterfrom
cdrini:refactor/add-get-db-type
Apr 28, 2026
Merged

Add types to _get_db#12462
mekarpeles merged 1 commit intointernetarchive:masterfrom
cdrini:refactor/add-get-db-type

Conversation

@cdrini
Copy link
Copy Markdown
Collaborator

@cdrini cdrini commented Apr 28, 2026

Note: This might cause more trouble than it's worth, since the webpy DB implementation isn't well-typed. But all the tests are passing...

Technical

Testing

Screenshot

Stakeholders

@cdrini cdrini marked this pull request as ready for review April 28, 2026 19:06
Copilot AI review requested due to automatic review settings April 28, 2026 19:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a type cast to _get_db() in openlibrary/core/db.py to make the cached DB accessor appear as a PostgresDB to type checkers (and imports the associated typing helpers).

Changes:

  • Import cast and PostgresDB.
  • Cast the return value of web.database(**web.config.db_parameters) to PostgresDB.

Comment thread openlibrary/core/db.py
Comment on lines 16 to +18
@functools.cache
def _get_db():
return web.database(**web.config.db_parameters)
return cast(PostgresDB, web.database(**web.config.db_parameters))
Copy link

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

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

_get_db() can return a SQLite-backed web.py DB in unit tests (e.g., tests set web.config.db_parameters = {"dbn": "sqlite", ...}), so casting the result of web.database(...) to PostgresDB is too narrow/misleading and can hide type errors (type checker will allow Postgres-only attributes even though runtime may be SQLite). Use a broader return type (common base/interface/Protocol or Union of supported DB types) and add an explicit return type annotation instead of forcing PostgresDB here.

Copilot uses AI. Check for mistakes.
@github-project-automation github-project-automation Bot moved this to Waiting Review/Merge from Staff in Ray's Project Apr 28, 2026
@mekarpeles mekarpeles merged commit 6a362e6 into internetarchive:master Apr 28, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from Waiting Review/Merge from Staff to Done in Ray's Project Apr 28, 2026
@mekarpeles mekarpeles assigned mekarpeles and unassigned RayBB Apr 28, 2026
mekarpeles added a commit that referenced this pull request Apr 28, 2026
@cdrini cdrini deleted the refactor/add-get-db-type branch April 28, 2026 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants