Opened 9 years ago
Closed 9 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)
Change History (19)
Changed 9 years ago by
Attachment: | merge-py3utils.patch added |
---|
comment:1 Changed 9 years ago by
Keywords: | review added |
---|
Changed 9 years ago by
Attachment: | merge-py3utils.2.patch added |
---|
comment:2 Changed 9 years ago by
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 9 years ago by
Attachment: | merge-py3utils-file-removed.patch added |
---|
files werent removed because i did rm instead of svn rm, fixed now
comment:3 Changed 9 years ago by
Author: | → tomprince |
---|---|
Branch: | → branches/internet-utils-mergs-6240 |
(In [37385]) Branching to internet-utils-mergs-6240.
comment:4 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Josh Schneier |
Thanks for your contribution.
- 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.
- That is, you should conventionalize the import of
- 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 oftwisted/python/util.py
andtwisted/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 9 years ago by
Attachment: | merge-py3utils-redux.patch added |
---|
comment:6 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Josh Schneier to Tom Prince |
Status: | new → assigned |
reraise
needs to be imported unconditionally. (Apparently there isn't test coverage for this unfortunately)admin/_twistedpython3.py
needs to be updated to include the merged files.- (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 9 years ago by
I'm confused, can you explain why?
- I looked through
twisted.internet.utils
and didn't find any hint ofreraise
why would I want to still import it in general? - 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.
- I tried to do just that. Unfortunately since I need to import
NativeStringIO
it isn't possible for me to keepStringIO.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 9 years ago by
First of all, let me say thanks for your patience and help. However, I'm confused, can you explain why?
- I looked through
twisted.internet.utils
and didn't find any hint ofreraise
why would I want to still import it in general?
- 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 9 years ago by
- I looked through twisted.internet.utils and didn't find any hint of reraise why would I want to still import it in general?
- 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 del
s 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 9 years ago by
- 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 9 years ago by
Keywords: | review added |
---|---|
Owner: | Tom Prince deleted |
Status: | assigned → new |
comment:12 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Tom Prince |
- I don't think you need to do "del ProcessUtilsTests", the tests will skip properly.
Looks good, please merge.
comment:13 Changed 9 years ago by
Branch: | branches/internet-utils-mergs-6240 → branches/internet-utils-mergs-6240-2 |
---|
(In [37873]) Branching to internet-utils-mergs-6240-2.
comment:14 Changed 9 years ago by
- 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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
NativeStringIO was having a method called on it, extra periods I didn't delete it