Skip to content
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

added SplashHtmlResponse (Fixed #114) #115

Closed
wants to merge 9 commits into from
Closed

added SplashHtmlResponse (Fixed #114) #115

wants to merge 9 commits into from

Conversation

atultherajput
Copy link

@atultherajput atultherajput commented Mar 27, 2017

Please guide me as this is my first PR @kmike and @redapple . I had tried to fix scrapy/scrapy#2673 by adding SplashHtmlResponse, subclassing HtmlResponse.

Fixes #114, fixes #92.

@kmike
Copy link
Member

kmike commented Mar 27, 2017

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-io
Copy link

codecov-io commented Mar 27, 2017

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% <ø> (ø) ⬆️
scrapy_splash/response.py 92.47% <100%> (+0.16%) ⬆️

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.

@atultherajput
Copy link
Author

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

@redapple
Copy link
Contributor

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.

@kmike
Copy link
Member

kmike commented Mar 27, 2017

@atultherajput it seems this change won't fix scrapy/scrapy#2673 - SplashHtmlResponse in this PR is still not a subclass of HtmlResponse. I think it should be a subclass of both SplashTextResponse and HtmlResponse.

@atultherajput
Copy link
Author

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

@atultherajput
Copy link
Author

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

@redapple
Copy link
Contributor

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

@kmike
Copy link
Member

kmike commented Jul 11, 2017

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

@iAnanich
Copy link
Contributor

iAnanich commented Jan 17, 2018

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
Copy link
Contributor

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

My PR: #160

@Gallaecio
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants