Skip to content
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

Fix/update type annotations #225

Merged
merged 4 commits into from
Feb 23, 2024
Merged

Fix/update type annotations #225

merged 4 commits into from
Feb 23, 2024

Conversation

alessio-locatelli
Copy link
Collaborator

@alessio-locatelli alessio-locatelli commented Feb 19, 2024

There were some tricky places where I had two choices: a major refactoring or a "best effort" attempt to fix the type annotations.

A simplified example to explain a challenge I had to solve in the "test/" folder:

class BaseStorageTest:
    storage_class: type[BaseCache]  # This is tricky because we assign child classes that have more different attributes.
   
   def init_cache() -> BaseCache:  # This is tricky because this is a base class, but we always return a child class (Mongo, Redis, SQLite, etc.)
       yield self.storage_class


class TestMongoDBCache(BaseStorageTest):
    storage_class: MongoDBCache
        
    # Here we have many warnings about `BaseCache` has no attribute [...] because `init_cache` says it returns `BaseCache`.
    # For example, `BaseCache` has not attribute named `collection`.


class TestSQLiteCache(BaseStorageTest):
    storage_class: SQLiteCache

    # Here we have many warnings about `BaseCache` has no attribute [...] because `init_cache` says it returns `BaseCache`.
    # Type checkers do not know abut `SQLiteCache` *unique* attributes, so we have no IDE autocompletion and warnings from linters.

I was thinking about how to get IDE autocompletion and fix Mypy/Pyright warnings and come up with as small refactorings as possible, but better ideas are welcome.
The workaround was to define that if we pass type[SQLiteCache] then we get SQLiteCache, etc.

Another tricky task was the fact that you return -> CachedResponse | ClientResponse but in the tests you always check attributes of CachedResponse (e.g. from_cache). The simplest workaround was using cast() to tell IDE and a type checker that we work in the test with the CachedResponse.


CI for 3.11 and 3.12 failed with

cache/.venv/bin/python: bad interpreter: No such file or directory
Error: Process completed with exit code 126.

✔️ Locally I successfully executed all tests for 3.8 and 3.12.

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 97.95918% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.75%. Comparing base (3aa2e5e) to head (5b54454).

Files Patch % Lines
aiohttp_client_cache/response.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   97.22%   97.75%   +0.52%     
==========================================
  Files          10       10              
  Lines        1010     1025      +15     
  Branches      171      173       +2     
==========================================
+ Hits          982     1002      +20     
+ Misses         18       16       -2     
+ Partials       10        7       -3     

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

Copy link
Member

@JWCook JWCook left a comment

Choose a reason for hiding this comment

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

These look like some nice improvements! There are a couple parts I'll need to look at more closely later (like some of the type: ignores), but I think this looks good to merge now.

For testing CachedResponse vs. ClientResponse attributes, cast() is fine, as well as assert isinstance(response, CachedResponse). For the bigger problem of how to inform the type checker that we have extra attributes set on ClientResponse, that is a bit tricky.

In requests-cache, I decided to make a wrapper class around requests.Response, and have both that and CachedReponse inherit from a parent class that defines the extra cache-related attributes. I also chose to modify new requests.Response objects in-place instead of creating new objects, to avoid messing with too many internal attributes that could potentially break in future versions.

We could do something similar here if you'd like, but I'm open to other suggestions. One alternative would be to always return CachedResponse objects. I think that had some disadvantages when it comes to handling streaming requests (with requests), but I'm not certain if that's also the case with aiohttp.

cache/.venv/bin/python: bad interpreter: No such file or directory

That's an issue I've seen before with GitHub Actions caches. No idea why, but it seems to happen more frequently with older caches. I manually cleared the caches for those interpreters and re-ran them, and they're working fine now. You should have permissions to do that as well (under Actions -> Caches), but let me know if you don't.

@JWCook JWCook merged commit c7d4f56 into requests-cache:main Feb 23, 2024
8 checks passed
@JWCook JWCook added this to the v0.12 milestone Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants