-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add some types to coverstore #11593
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
base: master
Are you sure you want to change the base?
Add some types to coverstore #11593
Conversation
404b7d6 to
1accf59
Compare
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 adds type annotations to several functions in the coverstore codebase to improve code clarity and type safety. The changes introduce type hints for function parameters and return types, and define two TypedDict-style classes (CoverDbDetails and PartialCoverDetails) to provide structure for cover detail objects.
- Added type annotations to utility functions (
ol_things,ol_get) and core cover handling functions - Introduced
CoverDbDetailsandPartialCoverDetailsclasses to type cover detail objects - Refactored variable naming in the
cover.GETmethod for better clarity (e.g.,value→cover_id)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| openlibrary/coverstore/utils.py | Added type hints to ol_things and ol_get functions |
| openlibrary/coverstore/db.py | Introduced CoverDbDetails class and added type annotation to details function |
| openlibrary/coverstore/coverlib.py | Added TYPE_CHECKING import and type hint to read_image function |
| openlibrary/coverstore/code.py | Added PartialCoverDetails class, type annotations to multiple functions, and refactored variable naming in cover.GET |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RayBB
left a comment
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.
I didn't test this locally but we need to fix the key string issue.
Otherwise the types themselves look right.
Also today I learned about the _ support for large numbers.
| def get_cover_id(olkeys): | ||
| class PartialCoverDetails(web.storage): | ||
| id: int | ||
| key: str |
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.
Key isn't a literal string of "key" it's dynamic
|
|
||
|
|
||
| def details(id): | ||
| class CoverDbDetails(web.storage): |
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.
This is a web storage because that's what the db returns.
Small refactor while trying to grok some of the coverstore code in response to coverstore investigation during high traffic event.
Technical
Testing
Screenshot
Stakeholders