Opened 2 years ago

Closed 2 years ago

#6079 enhancement closed fixed (fixed)

Port twisted.web.server to Python 3

Reported by: exarkun Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: web Keywords:
Cc: jknight Branch: branches/webserver-py3-6079
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

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

Change History (10)

comment:1 Changed 2 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/webserver-py3-6079

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

comment:3 Changed 2 years ago by itamarst

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

comment:4 Changed 2 years ago by itamar

  • Keywords review added
  • Owner set to exarkun

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 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

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 2 years ago by itamar

  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 2 years ago by itamarst

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

comment:8 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

comment:9 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

Great, thanks, please merge.

comment:10 Changed 2 years ago by itamarst

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

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

Author: itamar
Review: exarkun
Fixes: #6079

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

Note: See TracTickets for help on using tickets.