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

Do not try to decode body of Response #175

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

whalebot-helmsman
Copy link
Contributor

After discussion in #173
Decoding body of Response using only utf-8 is right way to fail. It is better to skip decoding at all.

There is two problems with integration tests at this moment - scrapy/scrapy#3210 (comment) and scrapy/scrapy#3210 (comment) . Considering this, no integration tests added

Fixing Integration tests requires effort separate from proposed changes

@codecov
Copy link

codecov bot commented Apr 16, 2018

Codecov Report

Merging #175 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
- Coverage   92.67%   92.66%   -0.02%     
==========================================
  Files           9        9              
  Lines         587      586       -1     
  Branches      118      118              
==========================================
- Hits          544      543       -1     
  Misses         21       21              
  Partials       22       22
Impacted Files Coverage Δ
scrapy_splash/response.py 94% <ø> (-0.06%) ⬇️

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 e40ca4f...b772360. Read the comment docs.

@Gallaecio
Copy link
Contributor

Gallaecio commented Aug 5, 2019

@whalebot-helmsman I’ve found this pull request while reviewing some old pull requests. What would the next steps be for this request?

@whalebot-helmsman
Copy link
Contributor Author

I think we should merge it. Any thoughts @kmike ?

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

Successfully merging this pull request may close these issues.

2 participants