Ticket #5903 enhancement closed fixed

Opened 20 months ago

Last modified 20 months ago

Port twisted.python.filepath to Python 3

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/filepath-python3-5903-4
(diff, github, buildbot, log)
Author: itamarst, exarkun Launchpad Bug:

Description

filepath should run on Python 3.3.

Change History

1

Changed 20 months ago by itamarst

  • branch set to branches/filepath-python3-5903
  • branch_author set to itamarst

(In [35340]) Branching to 'filepath-python3-5903'

2

Changed 20 months ago by itamar

  • keywords review added
  • owner changed from itamar to exarkun

This also includes a port of zippath. It depends on twisted.python.util and runtime which haven't yet been ported, but work sufficiently well to at least help the bootstrapping along.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/filepath-python3-5903 is running now.

3

Changed 20 months ago by itamar

Sorry, not twisted.python.util, but rather twisted.python.win32.

4

Changed 20 months ago by exarkun

  • keywords review removed
  • owner changed from exarkun to itamar

Thanks.

  1. The new comment in admin/_twistedpython3.py seems misleading and incorrect now.
  2. in test_compat.py, assertNativeString is missing a docstring
  3. There's no test coverage for the non-ASCII cases of nativeString. Perhaps there should be?
  4. in test_paths.py, test_zipArchiveRepr seems like a very boring test. Perhaps the test should actually literally encode the expected result? It's clear that %r will equal %r, but that doesn't tell me it equals something useful.
  5. `filepath.py
    1. Rather than copying the implementation of FancyEqMixin, it would be better to put it into a new module and share it between util.py and filepath.py via import.
    2. zope import belongs between stdlib and twisted imports
    3. _secureEnoughString is used to generate path components, so it is safe for it to always return bytes, isn't it?
    4. int(most things) might be almost as redundant as long(most things). The stat result seems to have all int attributes except for st_dev which is, for some reason, a long. Do we even care? Perhaps letting whatever the underlying type is pass through is sufficient.
  6. On a very general note, as long as we're burning all this time looking at every single line of Twisted, perhaps we should be taking note of specific improvements that would be desirable. There are a lot of terrible things I see in this branch, but they're existing terrible things that don't get directly in the way of porting. Still, it would be good to track that they exist, and a lot of them might make good introductory (aka "easy") tickets for new contributors. It wouldn't surprise me if we came away from a ticket like this one with 5 - 10 other new tickets for tiny things (for example, not using eval in test_permissionsShorthand).
  7. I'm confused by the addition of twisted/python/test/test_urlpath.py.

5

Changed 20 months ago by itamarst

(In [35359]) Address review comments 1-3. Refs #5903

6

Changed 20 months ago by itamarst

(In [35360]) Address review comment 5. Refs #5903

7

Changed 20 months ago by itamar

  • keywords review added
  • owner changed from itamar to exarkun
  1. I'm not sure what comment you mean exactly. I tried to improve it.
  2. Done.
  3. Done.
  4. Dropped - zippath is too difficult to do in this branch given zipfile only does Unicode on Python 3, so I am only porting filepath.
  5. Done.
  6. I opened #5908, but don't want to spend the time looking for more silliness.
  7. urlpath, like zippath, used to be tested test_paths. Both of them are now tested in separate modules to ease testing.
  8. FilePath is now bytes-only on Python 3.

8

Changed 20 months ago by exarkun

  • status changed from new to assigned

9

Changed 20 months ago by exarkun

  • owner changed from exarkun to itamar
  • status changed from assigned to new
  • keywords review removed
  1. twisted.python.compat.unicode should be documented, I think.
  2. There's some conflicts in admin/_twistedpython3.py
  3. twisted/python/_utilpy3.py should be added to _twistedpython3.py right?
  4. _stub_urandom documents its return type as str. bytes instead?
  5. I wonder what case _stub_armor is for. When does base64.urlsafe_b64encode not exist? Also, does _stub_armor work? It just does s.encode("hex"), which will only work on unicode in Python 3.3. It gets passed the output of sha1.digest, which seems to be bytes. I guess there's no test coverage for the stub case of _randomEnoughString?
  6. I assume test_zippath and test_urlpath are just moved code with no other changes?

10

Changed 20 months ago by itamarst

  • branch changed from branches/filepath-python3-5903 to branches/filepath-python3-5903-2

(In [35378]) Branching to 'filepath-python3-5903-2'

11

Changed 20 months ago by itamarst

  • branch changed from branches/filepath-python3-5903-2 to branches/filepath-python3-5903-3

(In [35379]) Branching to 'filepath-python3-5903-3'

12

Changed 20 months ago by itamar

  • owner changed from itamar to exarkun
  • keywords review added

Re 1, it's not clear how I can add docstring without subclassing or defining of a new function, both of which would break uses like isinstance, so I just added a comment.

I dropped the two stub functions that were only necessary for versions of Python older than 2.4.

_twistedpython3.py had both issues fixed.

And, yes, those two test modules are just moved code, nothing has been changed.

13

Changed 20 months ago by exarkun

  • keywords review removed
  • owner changed from exarkun to itamar

Re 1, it's not clear how I can add docstring without subclassing or defining of a new function, both of which would break uses like isinstance, so I just added a comment.

epytext has module docstring "@var" support, that's a good place for it.

I dropped the two stub functions that were only necessary for versions of Python older than 2.4. _twistedpython3.py had both issues fixed. And, yes, those two test modules are just moved code, nothing has been changed.

Excellent.

Unfortunately I noticed another problem:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/filepath-python3-5903-3

Even ignoring the expected red (and purple today, hooray), that doesn't look good. A Python 2.6 incompatibility?

14

Changed 20 months ago by itamarst

(In [35384]) Add @var, refs #5903

15

Changed 20 months ago by exarkun

(In [35387]) Use SynchronousTestCase on Python 2.x

refs #5903

16

Changed 20 months ago by exarkun

(In [35397]) Try resolving symlinks in the base part of the temporary path name, to avoid being confused on systems where /tmp is a symlink.

refs #5903

17

Changed 20 months ago by exarkun

(In [35398]) Nope, that doesn't avoid the confusion. Try using the current directory instead.

refs #5903

18

Changed 20 months ago by exarkun

(In [35400]) Fix import problems

refs #5903

19

Changed 20 months ago by exarkun

  • branch changed from branches/filepath-python3-5903-3 to branches/filepath-python3-5903-4
  • branch_author changed from itamarst to itamarst, exarkun

(In [35401]) Branching to 'filepath-python3-5903-4'

20

Changed 20 months ago by exarkun

Finally got a good buildbot run. Fixed the Python 2.6 incompatibility mentioned above, as well as one OS X incompatibility related to temp file allocation.

21

Changed 20 months ago by exarkun

(In [35403]) Put that @var back in, how did it get dropped?

refs #5903

22

Changed 20 months ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [35404]) Merge filepath-python3-5903-4

Author: itamar, exarkun Reviewer: exarkun Fixes: #5903

Port twisted.python.filepath and its minor dependencies to Python 3.x.

23

Changed 20 months ago by exarkun

  • status changed from closed to reopened
  • resolution fixed deleted

(In [35408]) Revert r35404 - Python 2.6 test suite regressions

A bunch of failures like these:

exceptions.AttributeError: FilePathTestCase object has no attribute addCleanup

Reopens: #5903

24

Changed 20 months ago by itamarst

  • status changed from reopened to closed
  • resolution set to fixed

(In [35410]) Merge filepath-python3-5903-3

Author: itamar, exarkun Reviewer: exarkun Fixes: #5903

Port and its minor dependencies to Python 3.x.

25

Changed 20 months ago by itamar

Version 4 of the branch apparently dropped a bunch of commits, and version 3 merged OK, and had all commits. I therefore merged version 3.

Note: See TracTickets for help on using tickets.