add SplashHtmlResponse (extended #115)#160
add SplashHtmlResponse (extended #115)#160iAnanich wants to merge 15 commits intoscrapy-plugins:masterfrom
Conversation
* new `test/test_response.py` file * new `test/test_response/test_response_types` test function `test_response_types` checks for compatibility of `Splash*Response` classes with Scrapy's `*Response` classes with `issubclass` func
* use it in `test_middlewares.py` for `test_splash_request` * use it inside `SplashMiddleware._change_response_class` method
* case when response is instance of any `Splash*` class in `splash_scrapy_text_response` tuple of pairs
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 92.66% 92.76% +0.09%
==========================================
Files 9 9
Lines 586 594 +8
Branches 118 120 +2
==========================================
+ Hits 543 551 +8
Misses 21 21
Partials 22 22
Continue to review full report at Codecov.
|
| from scrapy.exceptions import NotConfigured | ||
| from scrapy.http.headers import Headers | ||
| from scrapy.http.response.text import TextResponse | ||
| from scrapy.http.response.html import HtmlResponse |
There was a problem hiding this comment.
I don’t see where this import is used in your changes.
| from scrapy_splash.response import splash_scrapy_text_responses | ||
| splash_text_response_types = tuple(x for x, y in splash_scrapy_text_responses) | ||
|
|
||
| if not isinstance(response, (SplashResponse, splash_text_response_types)): |
There was a problem hiding this comment.
Your new SplashHtmlResponse class inherits from SplashTextResponse. Doesn’t that make these changes and some changes below unnecessary?
|
|
||
| splash_scrapy_content_types = ( | ||
| (SplashTextResponse, TextResponse, b'text/*'), | ||
| (SplashHtmlResponse, HtmlResponse, b'text/html'), |
There was a problem hiding this comment.
💄 Since these lines are not too long, and this variable is only used once, maybe the variable could be removed and its content moved below, inline?
| # check response post-processing | ||
| response = TextResponse("http://127.0.0.1:8050/render.html", | ||
| response = scrapy_response_type( | ||
| url="http://127.0.0.1:8050/render.html", |
There was a problem hiding this comment.
💄 Too much indentation, I would say.
| headers={b'Content-Type': b'text/html'}, | ||
| body=b"<html><body>Hello</body></html>") | ||
| headers={b'Content-Type': content_type}, | ||
| body=b"<html><body>Hello</body></html>", ) |
There was a problem hiding this comment.
💄 The added comma is not necessary.
It makes sense to add commas at the end if the closing parenthesis is on a different line, that allows to keep a clean diff when adding new parameters. But if you keep the closing parenthesis on the same line, there is no benefit to it.
| assert response2.body == b"<html><body>Hello</body></html>" | ||
| assert response2.css("body").extract_first() == "<body>Hello</body>" | ||
| assert response2.headers == {b'Content-Type': [b'text/html']} | ||
| assert response2.headers == {b'Content-Type': [content_type, ]} |
There was a problem hiding this comment.
💄 That comma is only needed for tuples, not lists.
There was a problem hiding this comment.
idk, it makes it looking more like a list rather an object.
|
For reference: Fixes #114 |
|
@iAnanich It’s been a while, any chance you will resume this work, or should we close? |
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 92.63% 92.76% +0.12%
==========================================
Files 9 9
Lines 584 594 +10
Branches 117 120 +3
==========================================
+ Hits 541 551 +10
Misses 21 21
Partials 22 22
Continue to review full report at Codecov.
|
|
@Gallaecio yeah, a long time. |
Please, have a look at my feedback above when you have time, see if you can address it. |
|
Is this being worked on? |
5 participants
Contains rebased @atultherejput commits from #115 with more test cases and little improvements.