Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5903 enhancement closed fixed (fixed)

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 (25)

comment:1 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/filepath-python3-5903

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

comment:2 Changed 2 years 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.

comment:3 Changed 2 years ago by itamar

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

comment:4 Changed 2 years 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.

comment:5 Changed 2 years ago by itamarst

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

comment:6 Changed 2 years ago by itamarst

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

comment:7 Changed 2 years 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.

comment:8 Changed 2 years ago by exarkun

  • Status changed from new to assigned

comment:9 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new
  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?

comment:10 Changed 2 years ago by itamarst

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

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

comment:11 Changed 2 years 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'

comment:12 Changed 2 years ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun

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.

comment:13 Changed 2 years 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?

comment:14 Changed 2 years ago by itamarst

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

comment:15 Changed 2 years ago by exarkun

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

refs #5903

comment:16 Changed 2 years 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

comment:17 Changed 2 years ago by exarkun

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

refs #5903

comment:18 Changed 2 years ago by exarkun

(In [35400]) Fix import problems

refs #5903

comment:19 Changed 2 years ago by exarkun

  • Author changed from itamarst to itamarst, exarkun
  • Branch changed from branches/filepath-python3-5903-3 to branches/filepath-python3-5903-4

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

comment:20 Changed 2 years 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.

comment:21 Changed 2 years ago by exarkun

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

refs #5903

comment:22 Changed 2 years ago by exarkun

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

(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.

comment:23 Changed 2 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

(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

comment:24 Changed 2 years ago by itamarst

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

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

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

Port and its minor dependencies to Python 3.x.

comment:25 Changed 2 years 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.