-
Notifications
You must be signed in to change notification settings - Fork 453
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
add SplashHtmlResponse (extended #115) #160
base: master
Are you sure you want to change the base?
add SplashHtmlResponse (extended #115) #160
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.
|
@@ -14,6 +14,7 @@ | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 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?
@@ -82,25 +94,26 @@ def test_splash_request(): | |||
assert json.loads(to_native_str(req2.body)) == expected_body | |||
|
|||
# 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 Too much indentation, I would say.
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.
agree
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 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 is not response | ||
assert response2.real_url == req2.url | ||
assert response2.url == req.url | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 That comma is only needed for tuples, not lists.
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.
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? |
Contains rebased @atultherejput commits from #115 with more test cases and little improvements.