Opened 2 years ago

Closed 2 years ago

#5897 enhancement closed fixed (fixed)

Port twisted.python.runtime to Python 3

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/runtime-python3-5897-2
(diff, github, buildbot, log)
Author: meissenPlate, exarkun Launchpad Bug:

Description

twisted.python.runtime should run on Python 3.

Attachments (4)

runtime.py.diff (1.5 KB) - added by meissenPlate 2 years ago.
Patch to make twisted.python.runtime usable with BOTH 2x and 3x.
test_runtime.py.diff (515 bytes) - added by meissenPlate 2 years ago.
A test that should fail if the unmodified twisted.python.runtime were to be run under 3x. (Since trial itself doesn't run under 3x I can only say that it passes under 2x trial.)
twisted_python_runtime_3x_compatibility.diff (2.8 KB) - added by meissenPlate 2 years ago.
3x compatibility patch for twisted.python.runtime, with changes to that module's tests included.
twisted_python_runtime_3x_compatibility_2.diff (2.9 KB) - added by meissenPlate 2 years ago.
3x compatibility patch for twisted.python.runtime, with changes to that module's tests included. Ignore the 3rd file.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 2 years ago by itamar

  • Milestone set to Python 3.3 Minimal

Changed 2 years ago by meissenPlate

Patch to make twisted.python.runtime usable with BOTH 2x and 3x.

Changed 2 years ago by meissenPlate

A test that should fail if the unmodified twisted.python.runtime were to be run under 3x. (Since trial itself doesn't run under 3x I can only say that it passes under 2x trial.)

comment:2 Changed 2 years ago by meissenPlate

Nothing too complex, in 3x the module "thread" is "_thread" and "_winreg" is "winreg", so we just need to be a little smart when we import them. I added a test that should fail if the OLD runtime.py were run under 3x Python, but since trial itself doesn't run under 3x I can't test that it fails under those conditions. The NEW runtime.py passes all its tests under 2x and can be run and used under both 2x and 3x.

Running 2to3 on the OLD runtime.py corrects the name of the winreg module, but NOT the name of the thread module (because of the way it's being loaded). Running 2to3 on the NEW runtime.py produces no changes.

Incidentally: I noticed this file uses a variable named "type" which is covering up a Python built in. It sort of bothers me but I left it alone because it isn't hurting anything.

comment:3 Changed 2 years ago by meissenPlate

  • Keywords review added

comment:4 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to meissenPlate

Thanks. There are a few problems here:

  1. Please attach all changes as a single patch file, not a patch file per modified source file.
  2. Using __import__ to deal with the name changes is gratuitous. import x as y would be perfectly sufficient.
  3. It would be better not to perform the Python version checks in multiple places, and ideally not inside functions that may be called repeatedly.
  4. The tests need to pass on Python 3. Since trial is not ported yet, please switch the tests to using the standard library unittests module, add a comment at that code site indicating it should be switched back again and reference a newly filed ticket for doing that work.
  5. The new unit test doesn't make any assertions, so it's not very useful. The best implementation of isWinNT that would make the test pass is pass.
  6. twisted.python.compat._PY3 is the preferred way to check whether the Python runtime is a 3.x version or not.
  7. An explicit decision needs to be made about the string type for all strings in this module. We are probably leaning towards leaving "" literals alone (that is, letting str in Python 2.x be str in Python 3.x), but the decision still needs to be finalized.

Thanks again.

Changed 2 years ago by meissenPlate

3x compatibility patch for twisted.python.runtime, with changes to that module's tests included.

comment:5 Changed 2 years ago by meissenPlate

  • Keywords review added

Sorry for being so slow, the source control is not very strong with meissenPlate.

Ignore the 3rd file, typing up this message made me realize I still needed to do a couple things.

1. Please attach all changes as a single patch file, not a patch file per modified source file.

  1. Using import to deal with the name changes is gratuitous. import x as y would be perfectly sufficient.
  2. twisted.python.compat._PY3 is the preferred way to check whether the Python runtime is a 3.x version or not.

Done. Sorry about that.

4. The tests need to pass on Python 3. Since trial is not ported yet, please switch the tests to using the standard library unittests module, add a comment at that code site indicating it should be switched back again and reference a newly filed ticket for doing that work.

Makes sense. I've changed twisted.python.test.test_runtime to import TestCase from unittest instead of trial, added a TODO, and added ticket #5919 "Use trial to run the tests for twisted.python.runtime" http://twistedmatrix.com/trac/ticket/5919

5. The new unit test doesn't make any assertions, so it's not very useful. The best implementation of isWinNT that would make the test pass is pass.

When I wrote this test what I was thinking that the value of the test would be that it would have caught one of the two compatibility problems that twisted.python.runtime had, so I wasn't thinking about making it as tight as I could. I took another look at it and was able to tighten it up some: asserting that the return value was either 1 or 0, and always 0 if the system isn't some sort of Windows.

7. An explicit decision needs to be made about the string type for all strings in this module. We are probably leaning towards leaving "" literals alone (that is, letting str in Python 2.x be str in Python 3.x), but the decision still needs to be finalized.

You're worried about the bytestring/unicode thing? I agree that this is something worth talking about. I think this file's strings are fine how they are?

  1. It would be better not to perform the Python version checks in multiple places, and ideally not inside functions that may be called repeatedly.

I pulled the choice between _thread and thread out of the function into the class. I could do the same with winreg vs _winreg but we would need to go back to import or imp.load() or something. Thoughts?

Changed 2 years ago by meissenPlate

3x compatibility patch for twisted.python.runtime, with changes to that module's tests included. Ignore the 3rd file.

comment:6 Changed 2 years ago by exarkun

  • Owner changed from meissenPlate to exarkun

comment:7 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/runtime-python3-5897

(In [35422]) Branching to 'runtime-python3-5897'

comment:8 Changed 2 years ago by exarkun

(In [35423]) Apply twisted_python_runtime_3x_compatibility_2.diff

refs #5897

comment:9 Changed 2 years ago by exarkun

  • Keywords review removed

Thanks!

  1. There is some trailing whitespace in the patch.
  2. The docstring for test_isWinNT doesn't need to use the possessive where it does and that docstring should be wrapped at 79 columns.
  3. Python 2.6 stdlib unittest.TestCase does not have assertIn, so we need to do it manually.
  4. There are still multiple checks for Python 3. This is a check that can be done at module initialization time, once.
  5. It appears there are no unit tests for supportsThreads.
  6. Now that twisted.python.runtime has been ported to Python 3, it needs to be added to admin/_twistedpython3.py (and its test module)

comment:10 Changed 2 years ago by exarkun

(In [35424]) Address review points

refs #5897

comment:11 Changed 2 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to itamar

comment:13 Changed 2 years ago by itamarst

  • Author changed from exarkun to itamarst, exarkun
  • Branch changed from branches/runtime-python3-5897 to branches/runtime-python3-5897-2

(In [35436]) Branching to 'runtime-python3-5897-2'

comment:14 Changed 2 years ago by itamar

  • Keywords review removed

Looks good; just needs merging forward and adding the __future__ imports, which I will do.

comment:15 Changed 2 years ago by itamar

Oh, and add news file. I also pointed at #5885 instead for using trial.

comment:16 Changed 2 years ago by itamar

  • Author changed from itamarst, exarkun to meissenPlate, exarkun

comment:17 Changed 2 years ago by itamarst

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

(In [35440]) Merge runtime-python3-5897-2

Author: meissenPlate, exarkun
Review: exarkun, itamar
Fixes: #5897

twisted.python.runtime now works on Python 3.

Note: See TracTickets for help on using tickets.