Aiohttp downloadhandler #6798
Aiohttp downloadhandler #6798
Conversation
idk some initial ideas but im a bum lowk so its not even the full implementation
looking much better
im lowkey happy with this just gotta test see if this is gibberish or not
…/scrapy into aiohttp-downloadhandler
|
This is interesting! I have several comments and I think we have more tests in |
| @@ -8,6 +8,7 @@ dynamic = ["version"] | |||
| description = "A high-level Web Crawling and Web Scraping framework" | |||
| dependencies = [ | |||
| "Twisted>=21.7.0", | |||
| "aiohttp>=3.11.18", | |||
There was a problem hiding this comment.
Let's keep this for now but I think this will be an optional dependency for some time.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #6798 +/- ##
==========================================
- Coverage 90.78% 90.77% -0.02%
==========================================
Files 164 165 +1
Lines 12381 12418 +37
Branches 1609 1610 +1
==========================================
+ Hits 11240 11272 +32
- Misses 834 836 +2
- Partials 307 310 +3
|
|
additionally some of the tests for py 3.11-3.13 failed on the first push for unix systems because there was a test that is skipped for windows. i added code that hopefully fixes it but im not running ubuntu or macos at this time so im not sure whether its fully fixed |
|
I think for the |
tested this on linux with with dockers slim images so hopefully the timeout is passed
|
im not sure how my code has an effect on the engine tests in the case that is failing atm but i will look into it |
|
That's just #6783 |
|
i thought maybe its some versioning thing because it fails on a lower python version, but ig the code is fine right? |
|
Yes, the problem is unrelated to the changes in this PR. |
|
this issue looks similar to that previous one i think? in both cases its something with task being pending it seems |
|
do you think its ready or should i change something else? |
|
As for the tests, I would wait until #6821 is merged, after which it should be easier to subclass test base classes. I don't expect this handler to pass all of them as e.g. dataloss and maxsize handling is not implemented, but it would be better to subclass all test classes and mark the unimplemented ones with xfail. |
|
I've enabled all existing tests for this handler and skipped ones that don't pass. |
|
Another thing to consider, although maybe also OK to address on a separate PR, is to both extend the default list of RETRY_EXCEPTIONS to include aiohttp ones, and to modify the code that loads them so that it silently ignores those that cannot be imported. Or maybe we could make the default value depend on installed dependencies, so that we do not need to change the load handling and silence anything. |
Fixes #6707
this aiohttp dh passes all tox envs and the http test and should be ready to use, but admittedly this is my first contribution so if this is missing some functionality or something needs to be changed or tested im happy to adjust always