Opened 4 years ago

Closed 4 years ago

Last modified 22 months ago

#6079 enhancement closed fixed (fixed)

Port twisted.web.server to Python 3

Reported by: Jean-Paul Calderone Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Python 3.3 Minimal
Component: web Keywords:
Cc: jknight Branch: branches/webserver-py3-6079
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description

This is much of the API for actually starting a Twisted Web server.

Change History (11)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: jknight added

comment:2 Changed 4 years ago by itamarst

Author: itamarst
Branch: branches/webserver-py3-6079

(In [36369]) Branching to 'webserver-py3-6079'

comment:3 Changed 4 years ago by itamarst

(In [36370]) Move twisted.web.util tests to the appropriate test module, pretty much unchanged. Refs #6079

comment:4 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: set to Jean-Paul Calderone

A partial port, but even so I did unfortunately did too much.

Some tests were moved unchanged into twisted.web.test.test_util.

A better name for this ticket may be "Make twisted.web.test.test_web pass on Python 3" - some of the tests are intended to be server tests, but required fixes in other modules in code that wasn't fully ported.

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/webserver-py3-6079

comment:5 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

Thanks. Hope your "b" key still works.

  1. twisted.web.iweb is listed in two places in _twistedpython3.py
  2. in twisted/web/http.py
    1. in stringToDatetime, there's a change to do nativeString(month.lower()) instead of just month.lower(). is nativeString really needed here? It looks like month is already a native string.
    2. is there a ticket, yet, for replacing StringTransport here with a more complete implementation (perhaps even one that could be shared with unit tests)? if not, maybe we should have one.
    3. '%s: %s\r\n' % ("Set-Cookie", cookie) seems silly. I wonder why this wasn't/isn't 'Set-Cookie: %s\r\n' % (cookie,). Sort of out of scope, but...
    4. in setHeader, there are two assert statements added. assert statements are against the law. Please turn these into proper type checks with unit tests.
    5. The logging change looks like it alters behavior. Was None a possible return value before? It looks like it may not be anymore.
  3. in twisted/web/server.py
    1. Why bother with the conditional for the escape import? Like in html.py, cgi.escape seems like it will work equally well on either version. Also of the import from html were to stay, it shouldn't be a relative import.
    2. for the change to list(map(unquote, self.path[1:].split(b'/'))), I guess a list comprehension might be better for Python 2? Rather than an extra list copy. otoh, slower function calls. So sort of a loss regardless, nevermind.
    3. I guess it's wrong to use nativeString on self.method, since self.method is input from the network. If someone sends non-ASCII, we'll get a decode error.
  4. twisted/web/test/test_util.py
    1. should import neither DummyRequest nor DummyChannel from test_web

Guess I might like to take one more look at this before it is merged into trunk.

comment:6 Changed 4 years ago by Itamar Turner-Trauring

  1. Done.
  2. Dealt with:
    1. Done.
    2. #6185.
    3. Done.
    4. #6186, it was mostly there for debugging.
    5. There was a reason I did it, but I'm not sure what it was anymore. Removed.
  3. Dealt with:
    1. The html import is an _absolute_ import, it's Python's html module, not ours. The reason it's html.escape is that cgi.escape is deprecated in Python 3.3.
      >>> from cgi import escape; escape("hello")
      __main__:1: DeprecationWarning: cgi.escape is deprecated, use html.escape instead
      'hello'
      
      I'll get rid of the microdom import, at least.
    2. Did nothing.
    3. Changed to decode as 'charmap'.
  4. Done.

comment:7 Changed 4 years ago by itamarst

(In [36407]) Address review comments. Refs #6079

comment:8 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone

comment:9 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

Great, thanks, please merge.

comment:10 Changed 4 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [36410]) Merge webserver-py3-6079.

Author: itamar Review: exarkun Fixes: #6079

twisted.web.server now works, somewhat, on Python 3.

comment:11 Changed 22 months ago by Adi Roiban

This also fixed #3561

Note: See TracTickets for help on using tickets.