-
Notifications
You must be signed in to change notification settings - Fork 451
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
added SplashHtmlResponse (Fixed #114) #115
Conversation
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
@@ 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
Continue to review full report at Codecov.
|
After 5 unsuccessful attempts i think finally i had patched my first fix. Thanks a lot @kmike :) |
Thanks @atultherajput ! |
@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. |
@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: |
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? |
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.