add SplashHtmlResponse (extended #115) #160

iAnanich · 2018-01-19T03:13:54Z

Contains rebased @atultherejput commits from #115 with more test cases and little improvements.

* 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 · 2018-01-19T15:23:53Z

Codecov Report

Merging #160 into master will increase coverage by 0.09%.
The diff coverage is 92.85%.

@@            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

Impacted Files	Coverage Δ
scrapy_splash/responsetypes.py	`100% <ø> (ø)`	⬆️
scrapy_splash/__init__.py	`100% <100%> (ø)`	⬆️
scrapy_splash/response.py	`94.23% <100%> (+0.17%)`	⬆️
scrapy_splash/middleware.py	`89.88% <88.88%> (+0.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fb8c41...6340dad. Read the comment docs.

Gallaecio · 2019-04-05T04:41:16Z

scrapy_splash/middleware.py

@@ -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


I don’t see where this import is used in your changes.

Gallaecio · 2019-04-05T04:42:54Z

scrapy_splash/middleware.py

+        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)):


Your new SplashHtmlResponse class inherits from SplashTextResponse. Doesn’t that make these changes and some changes below unnecessary?

Gallaecio · 2019-04-05T04:47:53Z

tests/test_middleware.py

+
+splash_scrapy_content_types = (
+    (SplashTextResponse, TextResponse, b'text/*'),
+    (SplashHtmlResponse, HtmlResponse, b'text/html'),


~~💄 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?~~

Gallaecio · 2019-04-05T04:48:24Z

tests/test_middleware.py

@@ -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",


💄 Too much indentation, I would say.

Gallaecio · 2019-04-05T04:49:53Z

tests/test_middleware.py

-                            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>", )


💄 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.

Gallaecio · 2019-04-05T04:50:29Z

tests/test_middleware.py

    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, ]}


💄 That comma is only needed for tuples, not lists.

idk, it makes it looking more like a list rather an object.

Gallaecio · 2019-04-05T04:54:08Z

For reference: Fixes #114

Gallaecio · 2019-08-05T14:01:26Z

@iAnanich It’s been a while, any chance you will resume this work, or should we close?

codecov-io · 2020-02-23T22:47:06Z

Codecov Report

Merging #160 into master will increase coverage by 0.12%.
The diff coverage is 92.85%.

@@            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

Impacted Files	Coverage Δ
scrapy_splash/responsetypes.py	`100% <ø> (ø)`	⬆️
scrapy_splash/__init__.py	`100% <100%> (ø)`	⬆️
scrapy_splash/response.py	`94.23% <100%> (+0.17%)`	⬆️
scrapy_splash/middleware.py	`89.88% <88.88%> (+0.19%)`	⬆️
scrapy_splash/utils.py	`97.01% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2aaa79d...6340dad. Read the comment docs.

iAnanich · 2020-02-24T10:29:09Z

@Gallaecio yeah, a long time.
So what needs to be done?
I don't remember how, but I got around the problem.

Gallaecio · 2020-02-25T23:06:12Z

So what needs to be done?

Please, have a look at my feedback above when you have time, see if you can address it.

lucaguarro · 2022-04-06T19:43:27Z

Is this being worked on?

atultherajput and others added 15 commits January 18, 2018 00:41

added SplashHtmlResponse

c140615

fixed test_middleware

d4c280a

fixed test_middleware1

9ff12e4

fixed response

67edaae

fixed response1

5a59297

fixed response2

7b1cbcf

added as subclass of HtmlResponse

d951b56

now support application/xhtml+xml and application/vnd.wap.xhtml+xml

d6584ec

removed unused import

4d0c305

add test for Splash*Response types:

b2be0d5

* 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

add SplashHtmlResponse to __init__.py

692f590

add response.splash_scrapy_text_responses tuple of pairs

268cbc1

* use it in `test_middlewares.py` for `test_splash_request` * use it inside `SplashMiddleware._change_response_class` method

fix SplashMiddleware._change_response_class:

f16d6d3

* case when response is instance of any `Splash*` class in `splash_scrapy_text_response` tuple of pairs

fix test_splash_request: pass content type too

1815f51

fix Python2.7 compatibility in SplashMiddleware._change_response_class

6340dad

iAnanich mentioned this pull request Jan 20, 2018

added SplashHtmlResponse (Fixed #114) #115

Closed

Gallaecio requested changes Apr 5, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add SplashHtmlResponse (extended #115) #160

add SplashHtmlResponse (extended #115) #160

iAnanich commented Jan 19, 2018

codecov bot commented Jan 19, 2018 •

edited

Gallaecio Apr 5, 2019

Gallaecio Apr 5, 2019

Gallaecio Apr 5, 2019 •

edited

Gallaecio Apr 5, 2019

iAnanich Mar 1, 2020

Gallaecio Apr 5, 2019

Gallaecio Apr 5, 2019

iAnanich Mar 1, 2020

Gallaecio commented Apr 5, 2019

Gallaecio commented Aug 5, 2019

codecov-io commented Feb 23, 2020

iAnanich commented Feb 24, 2020

Gallaecio commented Feb 25, 2020 •

edited

lucaguarro commented Apr 6, 2022

add SplashHtmlResponse (extended #115) #160

Are you sure you want to change the base?

add SplashHtmlResponse (extended #115) #160

Conversation

iAnanich commented Jan 19, 2018

codecov bot commented Jan 19, 2018 • edited

Codecov Report

Gallaecio Apr 5, 2019

Choose a reason for hiding this comment

Gallaecio Apr 5, 2019

Choose a reason for hiding this comment

Gallaecio Apr 5, 2019 • edited

Choose a reason for hiding this comment

Gallaecio Apr 5, 2019

Choose a reason for hiding this comment

iAnanich Mar 1, 2020

Choose a reason for hiding this comment

Gallaecio Apr 5, 2019

Choose a reason for hiding this comment

Gallaecio Apr 5, 2019

Choose a reason for hiding this comment

iAnanich Mar 1, 2020

Choose a reason for hiding this comment

Gallaecio commented Apr 5, 2019

Gallaecio commented Aug 5, 2019

codecov-io commented Feb 23, 2020

Codecov Report

iAnanich commented Feb 24, 2020

Gallaecio commented Feb 25, 2020 • edited

lucaguarro commented Apr 6, 2022

codecov bot commented Jan 19, 2018 •

edited

Gallaecio Apr 5, 2019 •

edited

Gallaecio commented Feb 25, 2020 •

edited