Opened 4 years ago

Closed 4 years ago

#6240 task closed fixed (fixed)

Merge `twisted/internet/_utilspy3.py` into `twisted/internet/utils.py`

Reported by: Jean-Paul Calderone Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/internet-utils-mergs-6240-2
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

Also merge the associated test module. See #6183.

Attachments (4)

merge-py3utils.patch (12.8 KB) - added by Josh Schneier 4 years ago.
merge-py3utils.2.patch (12.8 KB) - added by Josh Schneier 4 years ago.
NativeStringIO was having a method called on it, extra periods I didn't delete it
merge-py3utils-file-removed.patch (18.4 KB) - added by Josh Schneier 4 years ago.
files werent removed because i did rm instead of svn rm, fixed now
merge-py3utils-redux.patch (18.7 KB) - added by Josh Schneier 4 years ago.

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by Josh Schneier

Attachment: merge-py3utils.patch added

comment:1 Changed 4 years ago by Josh Schneier

Keywords: review added

Changed 4 years ago by Josh Schneier

Attachment: merge-py3utils.2.patch added

NativeStringIO was having a method called on it, extra periods I didn't delete it

comment:2 Changed 4 years ago by Josh Schneier

i'm not sure why the tests didn't pick up the failure but patch2 is more correct. (is there a way to remove previously submitted patches?)

Changed 4 years ago by Josh Schneier

files werent removed because i did rm instead of svn rm, fixed now

comment:3 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/internet-utils-mergs-6240

(In [37385]) Branching to internet-utils-mergs-6240.

comment:4 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Josh Schneier

Thanks for your contribution.

  1. You shouldn't try to port the rest of the module in this ticket, other than the minimal syntax changes to get the file to import.
    • That is, you should conventionalize the import of StringIO on _PY3, rather than changing the import.
  2. For the things that aren't ported, you need to make sure that they aren't accessible. See the if _PY3: blocks at the end of twisted/python/util.py and twisted/python/test/test_util.py to see how to do this.

Please resubmit for review after addressing the above points.

(Note for future reviewers: I've done the initial dance to restore history, as in #6235 but haven't applied the above patch)

Changed 4 years ago by Josh Schneier

Attachment: merge-py3utils-redux.patch added

comment:5 Changed 4 years ago by Josh Schneier

Keywords: review added

changes requested made

comment:6 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Josh Schneier to Tom Prince
Status: newassigned
  1. reraise needs to be imported unconditionally. (Apparently there isn't test coverage for this unfortunately)
  2. admin/_twistedpython3.py needs to be updated to include the merged files.
  3. (minor) Please keep the import like:
    #!
    try:
        import cStringIO as StringIO
    except ImportError:
        import StringIO
    
    (as it was before merging) and don't do any import on _PY3.

I'll fix these up and submit for review. (I want somebody to review the svn merge dance as well).

comment:7 Changed 4 years ago by Josh Schneier

I'm confused, can you explain why?

  1. I looked through twisted.internet.utils and didn't find any hint of reraise why would I want to still import it in general?
  2. I removed the _utilspy3 ones and didn't put the utils and test_iutils in. Per your last comment I shouldn't try and port the files as a whole, so I removed them from the ported list.
  3. I tried to do just that. Unfortunately since I need to import NativeStringIO it isn't possible for me to keep StringIO.StringIO which is why I did it the way I did. Is there a better way?

Thanks for your patience and help.

Also, is there a way I should go about submitting additional patches when the first ones are inadequate? The way I have been doing it seems suboptimal.

comment:8 Changed 4 years ago by Josh Schneier

First of all, let me say thanks for your patience and help. However, I'm confused, can you explain why?

  1. I looked through twisted.internet.utils and didn't find any hint of reraise why would I want to still import it in general?
  1. I removed the _utilspy3 ones and didn't put the utils and test_iutils in. Per your last comment I shouldn't try and port the files as a whole, so I removed them from the ported list.

comment:9 Changed 4 years ago by Tom Prince

  1. I looked through twisted.internet.utils and didn't find any hint of reraise why would I want to still import it in general?

https://twistedmatrix.com/trac/browser/branches/internet-utils-mergs-6240/twisted/internet/utils.py?rev=#L203

  1. I removed the _utilspy3 ones and didn't put the utils and test_iutils in. Per your last comment I shouldn't try and port the files as a whole, so I removed them from the ported list.

Well, some of it was ported, so the modules should get installed, and the tests run, which is the main purpose of admin/_twistedpython3.py. The dels at the end of those modules will remove any trace of the unported parts, leaving just the ported parts useable and testable on python 3.

comment:10 Changed 4 years ago by Tom Prince

  1. I tried to do just that. Unfortunately since I need to import NativeStringIO it isn't possible for me to keep StringIO.StringIO which is why I did it the way I did. Is there a better way?

Since that code never actually gets run (just parsed), it doesn't in fact need NativeStringIO. It will when that code gets ported, and the change you did is probably the right thing to do there. (What happens is the function/classes get defined, then get deleted before they are ever called.)

Also, is there a way I should go about submitting additional patches when the first ones are inadequate? The way I have been doing it seems suboptimal.

Suboptimal how? If your patches have been applied to a branch, you should base future patches off the tip of the branch. That is often the case, but I didn't do that here. (If you have further patches for this ticket, they should be against the branch).

comment:11 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted
Status: assignednew

comment:12 Changed 4 years ago by therve

Keywords: review removed
Owner: set to Tom Prince
  1. I don't think you need to do "del ProcessUtilsTests", the tests will skip properly.

Looks good, please merge.

comment:13 Changed 4 years ago by Tom Prince

Branch: branches/internet-utils-mergs-6240branches/internet-utils-mergs-6240-2

(In [37873]) Branching to internet-utils-mergs-6240-2.

comment:14 Changed 4 years ago by Tom Prince

  1. I don't think you need to do "del ProcessUtilsTests", the tests will skip properly.

The code they are testing hasn't been ported yet. Deleting the tests is the idiom that has been used to indicate that. In any case, the process support in the reactor will get ported to python3 before the process support in t.i.utils, which will cause these tests to start failing.

comment:15 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37876]) Merge internet-utils-mergs-6240-2: Merge twisted/internet/_utilspy3.py into twisted/internet/utils.py.

Author: genericnick, tom.prince Reviewers: tom.prince, therve Fixes: #6240

Note: See TracTickets for help on using tickets.