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

Fix #1828: make enable_compression work on HTTP/1.0 #1910

Merged
merged 10 commits into from
Jun 22, 2017

Conversation

hubo1016
Copy link
Contributor

@hubo1016 hubo1016 commented May 22, 2017

What do these changes do?

This pr fix #1828 i.e. generate a correctly compressed response for HTTP/1.0 requests.

Are there changes in behavior for the user?

Call enable_compression on Response class with body set to some bytes now generates a response with content-length instead of chunk-encoded response.

Related issue number

#1828

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

@codecov-io
Copy link

codecov-io commented May 22, 2017

Codecov Report

Merging #1910 into master will increase coverage by <.01%.
The diff coverage is 94.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1910      +/-   ##
==========================================
+ Coverage   97.05%   97.06%   +<.01%     
==========================================
  Files          38       38              
  Lines        7674     7695      +21     
  Branches     1339     1346       +7     
==========================================
+ Hits         7448     7469      +21     
- Misses        102      103       +1     
+ Partials      124      123       -1
Impacted Files Coverage Δ
aiohttp/web_response.py 98.71% <94.28%> (+0.07%) ⬆️

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 44ebcac...a634b57. Read the comment docs.

@asvetlov
Copy link
Member

Looks like you've committed an syntax error :)

@hubo1016 hubo1016 force-pushed the bugfix-issue1828 branch 2 times, most recently from 469bc6a to c670c3a Compare May 24, 2017 03:03
@hubo1016
Copy link
Contributor Author

@asvetlov fixed

@asvetlov
Copy link
Member

Codecov reports that your change is not fully covered by tests.
Albeit it's sometimes acceptable we want to keep our test coverage level.
Please run make cov-dev and fix missing cases by adding new tests.

@asvetlov asvetlov mentioned this pull request May 26, 2017
if version >= HttpVersion11:
writer.enable_chunking()
headers[TRANSFER_ENCODING] = 'chunked'
if CONTENT_LENGTH in headers:
Copy link
Contributor Author

@hubo1016 hubo1016 May 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch cannot be covered in current version because all the content_length implementations do not return None when there is a Content-Length header. But I think it is necessary in case there are other implementations in the future. We should not rely on the current implementations.

@hubo1016
Copy link
Contributor Author

@asvetlov Seems that codecov report is not updating, now it is reported as
aiohttp/web_response.py 383 4 186 3 99%

Among the missing statements and branches, only one statement and one branch is related to this pr. The reason is updated in the above comment.

@hubo1016
Copy link
Contributor Author

hubo1016 commented Jun 1, 2017

@asvetlov any response?

@asvetlov asvetlov mentioned this pull request Jun 19, 2017
5 tasks
@asvetlov
Copy link
Member

Please create a PR against master.
Also your change fails one test on master branch.

@hubo1016 hubo1016 changed the base branch from 2.0 to master June 21, 2017 02:13
@asvetlov
Copy link
Member

The last tiny change: move a record in CHANGES.txt to 2.3 version

@asvetlov
Copy link
Member

Ok, I'll update CHANGES in merging.

@asvetlov asvetlov merged commit 58a7a58 into aio-libs:master Jun 22, 2017
@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable_compression causes exception on HTTP/1.0
3 participants