scrapy-splash
scrapy-splash copied to clipboard
added SplashHtmlResponse (Fixed #114)
Please guide me as this is my first PR @kmike and @redapple . I had tried to fix https://github.com/scrapy/scrapy/issues/2673 by adding SplashHtmlResponse, subclassing HtmlResponse.
Fixes #114, fixes #92.
Hey @atultherajput,
Thanks for the fix! It looks like tests are failing because SplashHtmlResponse should be a subclass of SplashTextResponse, like HtmlResponse is a subclass of TextResponse.
Codecov Report
Merging #115 into master will increase coverage by
0.03%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
+ Coverage 89.63% 89.66% +0.03%
==========================================
Files 9 9
Lines 569 571 +2
Branches 114 114
==========================================
+ Hits 510 512 +2
Misses 30 30
Partials 29 29
Impacted Files | Coverage Δ | |
---|---|---|
scrapy_splash/responsetypes.py | 83.33% <ø> (ø) |
:arrow_up: |
scrapy_splash/response.py | 92.47% <100%> (+0.16%) |
:arrow_up: |
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 7a56b3c...6725240. Read the comment docs.
After 5 unsuccessful attempts i think finally i had patched my first fix. Thanks a lot @kmike :)
Thanks @atultherajput !
I think you can also set SplashHtmlResponse
for 'application/xhtml+xml'
and 'application/vnd.wap.xhtml+xml'
so as to follow what Scrapy does.
@atultherajput it seems this change won't fix https://github.com/scrapy/scrapy/issues/2673 - SplashHtmlResponse in this PR is still not a subclass of HtmlResponse. I think it should be a subclass of both SplashTextResponse and HtmlResponse.
@kmike please review patch again. I had made few changes.
Thanks @redapple for the suggestions. I had added set SplashHtmlResponse for 'application/xhtml+xml' and 'application/vnd.wap.xhtml+xml' and please do review my patch again and suggest if i am missing something. :)
@kmike , what do you think of this PR now?
@redapple I need to refresh my memory and check it again, but as I recall there is an issue with this PR, it didn't solve the problem, and that's why it was not merged. I should have commented with my findings back then.
I'm trying to resolve it in this branch. Is there a way to add my commits to this PR without interrupting to @atultherajput 's repository?
Somehow all integration tests fail: StopIteration
with AttributeError
for different Spider
instances that has no collected_items
attribute. Also posted this question on Stack Overflow because I have some problems with my Splash (again) and can't test properly.
Almost resolved my issue with rendering, and integration tests are not a problem too.
My PR: #160
@atultherajput It’s been a while, shall we close?