Opened 3 years ago

Closed 10 days 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
(github, patch, buildbot, log)
Author: adiroiban Launchpad Bug:


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 13 days ago.
headers-keyword-update-6082-2.patch (1.6 KB) - added by Unit03 11 days ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

Changed 13 days ago by Unit03

comment:2 Changed 13 days 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 13 days 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 13 days 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 12 days 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 12 days 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

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:

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


comment:7 Changed 11 days ago by adiroiban

  • Owner changed from adiroiban to Unit03

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


Changed 11 days ago by Unit03

comment:8 Changed 11 days 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 11 days ago by adiroiban

  • Keywords review removed
  • Owner changed from adiroiban to Unit03


Changes applied.

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

Many thanks again for your help!

comment:10 Changed 10 days 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.