Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#5903 enhancement closed fixed (fixed)

Port twisted.python.filepath to Python 3

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/filepath-python3-5903-4
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst, exarkun

Description

filepath should run on Python 3.3.

Change History (25)

comment:1 Changed 5 years ago by itamarst

Author: itamarst
Branch: branches/filepath-python3-5903

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

comment:2 Changed 5 years ago by Itamar Turner-Trauring

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

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 5 years ago by Itamar Turner-Trauring

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

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

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

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

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

comment:6 Changed 5 years ago by itamarst

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

comment:7 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone
  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 5 years ago by Jean-Paul Calderone

Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring
Status: assignednew
  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 5 years ago by itamarst

Branch: branches/filepath-python3-5903branches/filepath-python3-5903-2

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

comment:11 Changed 5 years ago by itamarst

Branch: branches/filepath-python3-5903-2branches/filepath-python3-5903-3

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

comment:12 Changed 5 years ago by Itamar Turner-Trauring

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

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 5 years ago by Jean-Paul Calderone

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

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

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

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

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

refs #5903

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

(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 5 years ago by Jean-Paul Calderone

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

refs #5903

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

(In [35400]) Fix import problems

refs #5903

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

Author: itamarstitamarst, exarkun
Branch: branches/filepath-python3-5903-3branches/filepath-python3-5903-4

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

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

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 5 years ago by Jean-Paul Calderone

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

refs #5903

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

Resolution: fixed
Status: newclosed

(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 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

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

Resolution: fixed
Status: reopenedclosed

(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 5 years ago by Itamar Turner-Trauring

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.