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

Don't convert bytes headers to str #1736

Merged
merged 1 commit into from Sep 5, 2018
Merged

Conversation

@tobiashenkel
Copy link
Contributor

@tobiashenkel tobiashenkel commented Sep 4, 2018

Don't convert bytes header values using str() as str(b'foo') becomes
"b'foo'". Instead just leave bytes as is like it was prior to v18.0.0.

  • What kind of change does this PR introduce?
    • bug fix
@webknjaz
Copy link
Member

@webknjaz webknjaz commented Sep 4, 2018

Fair enough. Can we have a test for that, please?

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Sep 4, 2018

Also, AppVeyor tests failed.

Don't convert bytes header values using str() as str(b'foo') becomes
"b'foo'". Instead just leave bytes as is like it was prior to v18.0.0.
@tobiashenkel tobiashenkel force-pushed the tobiashenkel:fix-encoding branch from 75e4cd2 to 802d020 Sep 4, 2018
@codecov
Copy link

@codecov codecov bot commented Sep 4, 2018

Codecov Report

Merging #1736 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
- Coverage   79.86%   79.84%   -0.03%     
==========================================
  Files         104      104              
  Lines       13337    13334       -3     
==========================================
- Hits        10652    10646       -6     
- Misses       2685     2688       +3
@tobiashenkel
Copy link
Contributor Author

@tobiashenkel tobiashenkel commented Sep 4, 2018

Sure, I added a test case that fails without the fix.

@tobiashenkel
Copy link
Contributor Author

@tobiashenkel tobiashenkel commented Sep 4, 2018

The AppVeyor test seems to fail due to some unrelated installation problem.

openstack-gerrit pushed a commit to openstack-infra/zuul that referenced this pull request Sep 4, 2018
With the latest cherrypy version the tests run into the hard test
timeout. This is caused by a header conversion issue introduced in
cherrypy 18.0.0. There is a pull request [1] that fixes this
issue. Once this is merged and released we can uncap it again.

The newest reno release, 2.10.0, requires a feature of newer sphinx without
specifying that the minimum compatibility has changed. Uncap sphinx here
so at least the doc build is not broken, use Sphinx 1.6.1 since that is
the oldest sphinx version supporting what reno needs.

[1] cherrypy/cherrypy#1736

Change-Id: Ic00e664152062d1fc1677e467b638ac2be99aef1
Co-Authored-By: Andreas Jaeger <jaegerandi@gmail.com>
Copy link
Member

@webknjaz webknjaz left a comment

Thank you!

@webknjaz webknjaz merged commit 4142bea into cherrypy:master Sep 5, 2018
3 of 5 checks passed
3 of 5 checks passed
LGTM analysis: Python Analysis Failed (could not build the base commit (ce51624))
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tobiashenkel tobiashenkel deleted the tobiashenkel:fix-encoding branch Sep 5, 2018
@webknjaz webknjaz added the regression label Sep 8, 2018
@jaraco
Copy link
Member

@jaraco jaraco commented Sep 8, 2018

This fix is going out as 18.0.1.

openstack-gerrit pushed a commit to openstack-infra/zuul that referenced this pull request Sep 27, 2018
18.0.1 has been released an fixes:

  cherrypy/cherrypy#1736

Change-Id: Iffda4376e1393020d54ba6e25d0dda07616d72c6
Signed-off-by: Paul Belanger <pabelanger@redhat.com>
h3llrais3r added a commit to h3llrais3r/cherrypy that referenced this pull request Nov 4, 2018
@h3llrais3r h3llrais3r mentioned this pull request Nov 4, 2018
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants