Opened 4 years ago

Closed 8 months ago

#6082 enhancement closed fixed (fixed)

Make test_updateWithKeywords work on Python 3

Reported by: exarkun Owned by: Unit03
Priority: normal Milestone: Python-3.x
Component: web Keywords:
Cc: jknight Branch: branches/http-headers-update-keywords-6082
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

twisted.web.test.test_http_headers._DictHeader.update works like dict.update, accepting keyword arguments. Since header names should be bytes, though, some extra support is needed to make it correct on Python 3.

Skipped porting this functionality as part of #6080 since it's somewhat obscure and slightly more work, but eventually it should be supported.

Attachments (2)

headers-keyword-update-6082-1.patch (1.2 KB) - added by Unit03 9 months ago.
headers-keyword-update-6082-2.patch (1.6 KB) - added by Unit03 8 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 9 months ago by Unit03

comment:2 Changed 9 months ago by Unit03

  • Keywords review added

This is seemingly more trivial than expected, but maybe I'm missing something.

t.w.http_headers._DictHeaders stores headers with keys as bytes, but for keyword-style update of the inner dict, keyword names - which are unicode/str - must first be encoded in __setitem__. I wasn't sure about which encoding to use.

Does the patch look reasonable?

comment:3 Changed 9 months ago by adiroiban

  • Author set to adiroiban
  • Branch set to branches/http-headers-update-keywords-6082

(In [46240]) Branching to http-headers-update-keywords-6082.

comment:4 Changed 9 months ago by adiroiban

  • Keywords review removed
  • Owner set to Unit03

Many thanks for your patch!

Is 'ascii' the right encoding for headers? From what I understand from RFC 2616 I think that latin1 characters are also allowed as part of the headers.

Looking forward for your comments.

I have applied your latest patch and sent it to our builbot.

In case latin1 is not supported, please add an explicit test and update the documentation to let user know that even if RFC 2616 allows latin1, Twisted does not yet support that.

Thanks again!

comment:5 Changed 9 months ago by Unit03

  • Owner changed from Unit03 to adiroiban

Then I guess it should be latin1. But

In case latin1 is not supported (...)

Not supported where?

comment:6 Changed 9 months ago by adiroiban

Not supported by Twisted.


I think that ASCII is fine. I was confused ... but now I think that latin-1 was only supported as header value, not as header name

https://tools.ietf.org/html/rfc7230#section-3.2.4

Historically, HTTP has allowed field content with text in the ISO-8859-1 charset [ISO-8859-1], supporting other charsets only through use of [RFC2047] encoding. In practice, most HTTP header field values use only a subset of the US-ASCII charset [USASCII]. Newly defined header fields SHOULD limit their field values to US-ASCII octets. A recipient SHOULD treat other octets in field content (obs-text) as opaque data.


So. Your patch is great.

It still needs a news file fragment so that we will have a note in the release note and people will know that this bug is fixed.

Here is some more info about how we manage the news file: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

For this case, feel fee to just the release notes as a comment and I will take care of creating the right file.

Thanks!

comment:7 Changed 8 months ago by adiroiban

  • Owner changed from adiroiban to Unit03

Please provide a text for the release notes associated with this patch.

Thanks!

Changed 8 months ago by Unit03

comment:8 Changed 8 months ago by Unit03

  • Keywords review added
  • Owner changed from Unit03 to adiroiban

Great, thanks. :)

Sorry about lack of news file, I wrongly assumed I'm not supposed to write it. I'm attaching updated patch with news entry.

Feel free to reword it if necessary, though (I wasn't sure about few things, e.g. whether to include that t.w.http_headers is now fully ported to 3 - it wasn't mentioned in NEWS previously).

comment:9 Changed 8 months ago by adiroiban

  • Keywords review removed
  • Owner changed from adiroiban to Unit03

Thanks.

Changes applied.

Will wait for test result and if all is ok will merge.

Many thanks again for your help!

comment:10 Changed 8 months ago by adiroiban

  • Resolution set to fixed
  • Status changed from new to closed

(In [46255]) Merge http-headers-update-keywords-6082: Fix updating twisted.web.http_headers._DictHeaders using keywords in Python 3.

Author: Unit03 Reviewer: adiroiban Fixes: #6082

Note: See TracTickets for help on using tickets.