Opened 7 years ago
Closed 7 years ago
#7874 enhancement closed fixed (fixed)
twisted.internet.unix should be ported to Python 3
Reported by: | hawkowl | Owned by: | hawkowl |
---|---|---|---|
Priority: | normal | Milestone: | Python-3.x |
Component: | core | Keywords: | |
Cc: | Branch: |
branches/tiunix-py3-7874-2
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | hawkowl |
Description
Change History (18)
comment:1 Changed 7 years ago by
Author: | → hawkowl |
---|---|
Branch: | → branches/tiunix-py3-7874 |
comment:2 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | hawkowl deleted |
Changes done, builders spun and green, please review.
comment:3 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to hawkowl |
Changes looks good. Many thanks!
For test_reprWithClassicProtocol.skip and test_reprWithClassicFactory.skip I would like to see them closer to the initial code.. and not after the NewClass tests.
I don't know what are the plans for t.python.FilePath but I think that we should avoid using private stuff from it...
Why do we need the implicit conversion of the name member? Why convert on get and not on set?
For the other APIs we ask users to explicitly pass the right type, why we can not do the same for the new code.
In case we need the convesion I think that the docstring should explain why we need to conversion rather than describing in works what the actual code does :)
Thanks!
comment:4 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | hawkowl deleted |
Thanks for the review.
- I moved them to be closer.
- I think using the private methods is acceptable -- it's functionality that we need here, and we can't use the public methods because FilePath does more than just converting the encoding.
- It does convert on set.
- I added a comment about the passing being either text or bytes -- it asks for a filename, which is commonly Unicode on Python 3.
Builders spun, please review.
comment:5 Changed 7 years ago by
Thanks!
I think that at least md5(FilePath(case.mktemp())._asBytesPath()).hexdigest()
can be converted to use public API and not the private _asBytesPath.
I don't make sense of this comment ... sorry
On Linux, paths are always bytes. However, as paths are L{unicode} on Python 3, and this technically takes a file path, we convert it to bytes if need be to maintain compatibility with C{os.path} on Python 3.
Paths are Unicode on Windows and OSX... on Unix they are bytes even in Python3... is just that Python3 provide high level support to handle them as Unicode
Python 3.4.0 (default, Apr 11 2014, 13:05:11) [GCC 4.8.2] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import os >>> os.listdir('.') ['remote_put', 'test_filuțu', 'remote_get'] >>> os.listdir(b'.') [b'remote_put', b'test_filu\xc8\x9bu', b'remote_get']
and
we convert it to bytes if need be to maintain compatibility with
I have no idea what is expressed here.
I am leaving this in the review queue so that another reviewer can check it... I will be away in the next 2 weeks.
comment:6 Changed 7 years ago by
I think using the private methods is acceptable -- it's functionality that we need here, and we can't use the public methods because FilePath does more than just converting the encoding.
You don't need it. The function returns a random native string. How about using an actual random source instead of the md5 digest of a random path instead?
comment:7 Changed 7 years ago by
- I made it a MD5 of some os.urandom. Open to some better options.
- I ported over sendmsg, so that works on Python 3 now. It uses
socket.socket.sendmsg
instead oftwisted.python.sendmsg
. - Fixed up that comment to make a little more sense.
Builders are green.
comment:8 Changed 7 years ago by
What's the point of doing the MD5 digest of some random bytes? Just use the random bytes. If you want them to be human readable, hex encode them.
comment:9 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to hawkowl |
I ported over sendmsg
It looks more like you didn't port sendmsg. You ported some application code so that it conditionally uses our sendmsg module on Python 2 and the stdlib sendmsg functionality on Python 3. This makes the application code much more complicated. Instead, you should port the sendmsg module to Python 3 (it's a dependency of this functionality, it should have been ported first - that is (and has always been) the stated plan). Or, if the stdlib functionality is actually equivalent (have you verified that?) then you should introduce a separate compatibility layer that provides this conditional behavior and use that compatibility layer in application code.
comment:10 Changed 7 years ago by
Branch: | branches/tiunix-py3-7874 → branches/tiunix-py3-7874-2 |
---|
(In [44912]) Branching to tiunix-py3-7874-2.
comment:11 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | hawkowl deleted |
I have ported the changes in the old branch to use twisted.python.sendmsg
.
Builders spun, looking green so far, please review.
comment:12 Changed 7 years ago by
Changes look good... but I see that tests are failing. Thanks!
I think that we should remove/update this comment.
# Use the test cases's mktemp to get something unique...
A drive by comment. In twisted/internet/unix.py
the module docstring is Various asynchronous TCP/IP classes.
but I see UDP and Unix domain socket handling.
Waiting for buildbots to get green and will do a new review.
comment:13 Changed 7 years ago by
if requireModule("twisted.python.sendmsg"):
should be
if requireModule("twisted.python.sendmsg") is not None:
comment:14 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to hawkowl |
Hi HawkOwl,
Thanks for working on this!
Some things that should be addressed:
- "On Linux, paths are always bytes." -- Twisted does support non-Linux UNIXs. What happens on FreeBSD? This docstring is confusing.
- "self.assertTrue(UNIXAddress(self._socketAddress) == UNIXAddress(linkName))" If they really can't use assertEquals
- Add a comment why
- Add an explicit message
One thing you might want to consider doing differently:
- Different behavior on an edge case -- "self._name = _asFilesystemBytes(name) if name else None" means we turn empty string into None. Different exceptions will be raised?
Please address and resubmit.
comment:16 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | hawkowl deleted |
Thanks for the review, Adi and Moshe :)
I've addressed all the comments, and shoved the builders to do another run. Please review.
comment:18 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [44591]) Branching to tiunix-py3-7874.