Opened 2 years ago

Closed 20 months ago

#6240 task closed fixed (fixed)

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

Reported by: exarkun Owned by: tom.prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/internet-utils-mergs-6240-2
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

Description

Also merge the associated test module. See #6183.

Attachments (4)

merge-py3utils.patch (12.8 KB) - added by dreamriver 21 months ago.
merge-py3utils.2.patch (12.8 KB) - added by dreamriver 21 months 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 dreamriver 21 months ago.
files werent removed because i did rm instead of svn rm, fixed now
merge-py3utils-redux.patch (18.7 KB) - added by genericnick 21 months ago.

Download all attachments as: .zip

Change History (19)

Changed 21 months ago by dreamriver

comment:1 Changed 21 months ago by dreamriver

  • Keywords review added

Changed 21 months ago by dreamriver

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

comment:2 Changed 21 months ago by dreamriver

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 21 months ago by dreamriver

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

comment:3 Changed 21 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/internet-utils-mergs-6240

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

comment:4 Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to genericnick

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 21 months ago by genericnick

comment:5 Changed 21 months ago by genericnick

  • Keywords review added

changes requested made

comment:6 Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner changed from genericnick to tom.prince
  • Status changed from new to assigned
  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 21 months ago by genericnick

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 21 months ago by genericnick

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 21 months 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 21 months 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 21 months ago by tom.prince

  • Keywords review added
  • Owner tom.prince deleted
  • Status changed from assigned to new

comment:12 Changed 20 months 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 20 months ago by tomprince

  • Branch changed from branches/internet-utils-mergs-6240 to branches/internet-utils-mergs-6240-2

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

comment:14 Changed 20 months 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 20 months ago by tomprince

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

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