scrapy-splash icon indicating copy to clipboard operation
scrapy-splash copied to clipboard

added SplashHtmlResponse (Fixed #114)

Open atultherajput opened this issue 7 years ago • 12 comments

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.

atultherajput avatar Mar 27 '17 08:03 atultherajput

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.

kmike avatar Mar 27 '17 09:03 kmike

Codecov Report

Merging #115 into master will increase coverage by 0.03%. The diff coverage is 100%.

Impacted file tree graph

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

codecov-io avatar Mar 27 '17 10:03 codecov-io

After 5 unsuccessful attempts i think finally i had patched my first fix. Thanks a lot @kmike :)

atultherajput avatar Mar 27 '17 10:03 atultherajput

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.

redapple avatar Mar 27 '17 10:03 redapple

@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 avatar Mar 27 '17 10:03 kmike

@kmike please review patch again. I had made few changes.

atultherajput avatar Mar 28 '17 04:03 atultherajput

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

atultherajput avatar Mar 28 '17 04:03 atultherajput

@kmike , what do you think of this PR now?

redapple avatar Jul 11 '17 14:07 redapple

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

kmike avatar Jul 11 '17 17:07 kmike

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.

iAnanich avatar Jan 17 '18 23:01 iAnanich

Almost resolved my issue with rendering, and integration tests are not a problem too.

My PR: #160

iAnanich avatar Jan 20 '18 06:01 iAnanich

@atultherajput It’s been a while, shall we close?

Gallaecio avatar Aug 05 '19 14:08 Gallaecio