Opened 6 years ago
Closed 5 years ago
#5387 enhancement closed fixed (fixed)
Get rid of references and code specific to Python 2.4
Reported by: | jesstess | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | jesstess, Bartek, Thijs Triemstra | Branch: |
branches/remove-py24-refs-5387-4
branch-diff, diff-cov, branch-cov, buildbot |
Author: | bartekci, MostAwesomeDude, thijs |
Description (last modified by )
Once we drop support for 2.4 in ticket 4962 and close #5385 (py2.2) and #5386 (py2.3). Some of these comments can just get removed. Some might require removal of code as well.
twisted/conch/test/test_filetransfer.py: # In Python 2.4, the bad import has already been cleaned up for us. twisted/internet/defer.py: WARNING: this function will not work in Python 2.4 and earlier! twisted/internet/defer.py: generators. If you need to be compatible with Python 2.4 or before, use twisted/internet/unix.py: # Abstract namespace sockets aren't well supported on Python 2.4. twisted/python/dist.py: older versions (currently the only supported older version is 2.4), checks twisted/python/filepath.py: Provide random data in versions of Python prior to 2.4. This is an twisted/python/filepath.py: # Under Python 2.4 on Windows, WindowsError only has an errno twisted/python/filepath.py: # in Python 2.4 will result in a Windows error code of twisted/python/reflect.py: if (execName is None or # python 2.4+, post-cleanup twisted/python/test/test_dist.py: Tests for L{_checkCPython} when used on a Python 2.4-like platform, when twisted/python/test/test_zipstream.py: # In python 2.4 and earlier, consistency between the directory and the twisted/python/util.py: This is mostly necessary in Python 2.4 which implements L{id} to sometimes twisted/test/test_defgen.py:## syntax error in Python 2.4 and before. twisted/trial/runner.py: # objects do not have nice looking string formats on Python 2.4. twisted/trial/test/mockdoctest.py:# to test trial's doctest support with python2.4 twisted/trial/unittest.py: # in __call__, whereas 2.4 defines the code in run. twisted/trial/util.py: else: # use hotshot, profile is broken in 2.4 twisted/trial/util.py: # Python 2.4 has fixed this. Yay! twisted/web/client.py: # Python 2.4 doesn't have these symbolic constants twisted/web/test/test_xmlrpc.py: Tests for the C{useDateTime} flag on Python 2.4. twisted/web/xmlrpc.py:# On Python 2.4 and earlier, DateTime.decode returns unicode.
Attachments (3)
Change History (41)
comment:1 Changed 6 years ago by
Keywords: | easy added |
---|
comment:2 Changed 6 years ago by
comment:3 Changed 6 years ago by
Owner: | set to Bartek |
---|---|
Status: | new → assigned |
Changed 6 years ago by
Attachment: | ticket-5387.diff added |
---|
Initial work for review. Partially done deprecation
comment:4 Changed 6 years ago by
Initial work, still in progress, but would be happy if someone reviewed it to make sure it's appropriately done. Here's a summary of changes done so far:
twisted/web/test/test_xmlrpc.py -- Removed Python 2.4 specific test and flags.
twisted/web/xmlrpc.py -- Removed < Python 2.5 check in setattr -- Removed hack for DateTime.decode
twisted/web/client.py -- Adjusted getattr(os, 'SEEK_SET', 0) to os.SEEK_SET, kept private vclass variable though. Necessary?
twisted/trial/util.py -- Removed 2.4 hack under profiled -- DIDNOTFIX: Python 2.4 has fixed this, line 286. del sys.modules try/catch l
twisted/trial/unittest.py -- Removed overrides to call and run. Python 2.5/6 handle this as implemented.
twisted/trial/test/mockdoctest.py -- Removed reference to python 2.4 in doc, but test seems valid.
twisted/trial/runner.py -- Removed deprecated DocTestCase.
twisted/test/test_defgen.py -- Removed string exec hack for InlineCallbacksTests
comment:5 Changed 6 years ago by
Author: | → bartekci |
---|---|
Cc: | Bartek added |
Keywords: | review added |
Owner: | Bartek deleted |
Status: | assigned → new |
Thanks for this initial work, bartekci! If you want your work reviewed, please add the 'review' keyword and assign the ticket to <blank>. This will make it show up on http://twistedmatrix.com/trac/report/11 and in the review reminders on IRC. Otherwise it's likely to get lost in the shuffle. :) I'll go ahead and set it now.
Changed 6 years ago by
Attachment: | 5387.patch added |
---|
Added complete changes for ticket and news files. A few items referenced in the original ticket were left unchanged as they seemed rellevant to the codebase past Python 2.4
comment:6 Changed 6 years ago by
Description: | modified (diff) |
---|
(Just updating markup, please ignore.)
comment:7 Changed 6 years ago by
Author: | bartekci → bartekci, MostAwesomeDude |
---|---|
Branch: | → branches/remove-py24-refs-5387 |
(In [33806]) Branching to 'remove-py24-refs-5387'
comment:10 Changed 6 years ago by
Keywords: | easy, review → easy review |
---|
Looks like I've added myself to the authors list. Whatever.
This largely looks decent. The topfiles are not okay. I don't know what "Tornadio" is, but I'm pretty sure you meant something else. There are also spelling mistakes ("byres") which should be corrected.
I'm going to kick off a build for this branch; you can follow along at home at http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-py24-refs-5387
I know I'm forfeiting points, but I'm going to leave on the review keyword to attract other eyes. I don't see any code problems, but this is a very wide-reaching patch.
comment:11 Changed 6 years ago by
Keywords: | easy review removed |
---|
I only have some minor stylistic comments.
- In
python/dist.py
removing the Python 2.4 comment in the docstring leaves a partial line. I'd prefer the paragraph be reflowed to look tidier. - In
internet/defer.py
all references to Python 2.4 have been removed, but the reference to "Python 2.5 generators" is still there. Since we no longer support any version of Python /without/ generators, I'd drop the version from that sentence. - In
trial/test/mockdoctest.py
there is a reference to a__test__
attribute that doesn't seem to exist. This isn't strictly part of this ticket, but I think it can be cleaned up as well.
The code changes look okay. There's only one test failure on my system (some XPath thing in twisted.words), and that looks unrelated to the changes in this branch. (The same test fails in trunk.)
Ran 7295 tests in 121.716s FAILED (skips=1468, expectedFailures=11, failures=1, successes=5815)
comment:12 Changed 6 years ago by
Owner: | set to Corbin Simpson |
---|
comment:13 Changed 6 years ago by
There is also some version-specific code (for before Python 2.3.2) in twisted.words.protocols.jabber.xmpp_stringprep
.
Also, I think your failing test is in test_sip
, not xish.
Changed 6 years ago by
Attachment: | 5387.2.patch added |
---|
Updated patch with newsfile typo fixes and additional doc changes as requested.
comment:14 Changed 6 years ago by
Keywords: | review added |
---|
comment:15 Changed 6 years ago by
Owner: | changed from Corbin Simpson to rockstar |
---|
comment:17 Changed 6 years ago by
Updated the branch to the latest stuff from the patch. Please review.
bartekci, thank you for your hard work on this issue.
comment:18 Changed 6 years ago by
Keywords: | review removed |
---|---|
Owner: | rockstar deleted |
Hi bartekci, thanks for this patch.
Since you've removed DocTestCase here, you should probably also assign #5554 to yourself and close it when this ticket makes it to trunk.
After talking with dreid, I think it's probably better to ask that we hold off on completion of this ticket until after we close #5385 and #5386. A large reason is that there are many things like in t.internet.unix._UNIXPort that do things like "if sys.version_info > (2, 5)", and so we should probably remove that support in this ticket, *but* we should make sure that everything older than 2.4 is also taken care of.
comment:19 Changed 6 years ago by
Cc: | Thijs Triemstra added |
---|---|
Description: | modified (diff) |
comment:20 Changed 5 years ago by
Owner: | set to Thijs Triemstra |
---|---|
Status: | new → assigned |
comment:21 Changed 5 years ago by
Branch: | branches/remove-py24-refs-5387 → branches/remove-py24-refs-5387-2 |
---|
comment:22 Changed 5 years ago by
comment:24 Changed 5 years ago by
Author: | bartekci, MostAwesomeDude → bartekci, MostAwesomeDude, thijs |
---|---|
Branch: | branches/remove-py24-refs-5387-2 → branches/remove-py24-refs-5387-3 |
(In [37920]) Branching to 'remove-py24-refs-5387-3'
comment:26 Changed 5 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | assigned → new |
Forced a build, up for review.
comment:27 follow-up: 34 Changed 5 years ago by
Should the reference & conditional rockstar mentioned in t.i.unix._UNIXPort now be removed?
comment:28 Changed 5 years ago by
Owner: | set to Tom Prince |
---|---|
Status: | new → assigned |
comment:29 follow-up: 33 Changed 5 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Tom Prince to Thijs Triemstra |
Status: | assigned → new |
twisted/conch/topfiles/setup.py
: Do we even need the conditional anymore? If the only versions we support accept classifiers, we might as well always provide them.- This is in fact true of every
setup.py
.
- This is in fact true of every
twisted.internet.defer
: Don't delete the example.twisted.python.filepath
- Why are you removing the reference to randomBytes? If it is for compatibility, it should be deprecated in a separate ticket.
- The code in
children
can be simplified (but perhaps this should entirely be a separate ticket:.winerror
is always present now.- The comment suggests that it might be possible to simply use the posix error code on windows as well.
- Both of those changes probably require test changes.
twisted/python/test/test_dist.py
:- I think the comment about being tests for a 2.4-like platform are still accurate.
- I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.
twisted/trial/runner.py
:- The comment you are deleting suggests that we could simplify the code now. If this is the case, we should leave some evidence of this (new ticket?).
twisted/web/client.py
:- Since these constans are always present, we can simply use them directly, rather than the aliases you are touching.
twisted/web/xmlrpc.py
:- The
__setattr__
is now superfluous. - (minor)
L{xmlrpclib}
instead ofC{xmlrpclib}
.
- The
- There appears to be an error on the
py-select-gc
builder. - I haven't looked at coverage of the changed code, but this makes me thing that at least some of it isn't.
comment:31 Changed 5 years ago by
Branch: | branches/remove-py24-refs-5387-3 → branches/remove-py24-refs-5387-4 |
---|
(In [38652]) Branching to 'remove-py24-refs-5387-4'
comment:33 Changed 5 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Thanks for your review!
Replying to tom.prince:
twisted/conch/topfiles/setup.py
: Do we even need the conditional anymore? If the only versions we support accept classifiers, we might as well always provide them.
- This is in fact true of every
setup.py
.
Removed the conditionals in setup.py's.
twisted.internet.defer
: Don't delete the example.
Fixed.
twisted.python.filepath
- Why are you removing the reference to randomBytes? If it is for compatibility, it should be deprecated in a separate ticket.
This was part of a patch supplied by someone else; restored the reference again.
- The code in
children
can be simplified (but perhaps this should entirely be a separate ticket:
.winerror
is always present now.- The comment suggests that it might be possible to simply use the posix error code on windows as well.
- Both of those changes probably require test changes.
I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc.
twisted/python/test/test_dist.py
:
- I think the comment about being tests for a 2.4-like platform are still accurate.
Restored the comment.
- I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.
What do you suggest, should we rename _checkCPythonWithEmptyPlatform
to _checkPythonWithEmptyPlatform
? And/or remove the test_cpython
testcase?
twisted/trial/runner.py
:
- The comment you are deleting suggests that we could simplify the code now. If this is the case, we should leave some evidence of this (new ticket?).
Yes, I simplified it and it still passes the test. I think opening a new ticket will only create more work since its already covered by a test.. thoughts?
twisted/web/client.py
:
- Since these constans are always present, we can simply use them directly, rather than the aliases you are touching.
Fixed.
twisted/web/xmlrpc.py
:
- The
__setattr__
is now superfluous.- (minor)
L{xmlrpclib}
instead ofC{xmlrpclib}
.
Fixed.
- There appears to be an error on the
py-select-gc
builder.
Forced a new build
- I haven't looked at coverage of the changed code, but this makes me thing that at least some of it isn't.
The coverage report i generated shows (the changes made to) Proxy
is covered.
comment:34 Changed 5 years ago by
comment:35 Changed 5 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
twisted/trial/runner.py
:"%s" % (repr(x),)
is the same as"%r" % (x,)
I wonder, though, if we should be claiming that python is cpython in the cases described by the tests. None of 2.5, 2.6 or 2.7 fall into this case, so anything that we might conceivably support that behaves this way can't be cpython.
What do you suggest, should we rename
_checkCPythonWithEmptyPlatform
to_checkPythonWithEmptyPlatform
? And/or remove thetest_cpython
testcase?
I'm suggesting that
_checkCPython
should returnFalse
in the fallthrough, instead ofTrue
, since now that we don't support <=2.4, that case can't be cpython. This could be done in a seperate ticket.
I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc
Sure.
Please merge, after after addressing 1, and filing tickets for 2+3.
comment:37 Changed 5 years ago by
Status: | new → assigned |
---|
Replying to tom.prince:
twisted/trial/runner.py
:"%s" % (repr(x),)
is the same as"%r" % (x,)
Fixed.
I tried simplifying it but after several tries on the windows buildslave this is what worked. I'd prefer to handle this in a different ticket, i dont have access to a local windows machine for proper testing etc
Sure.
Looks somebody opened a ticket for this already (#6627) that includes a patch that seems to have a better fix as well.
Forced a [buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-py24-refs-5387-4 build] and will merge when it's green (after I opened a ticket for 2).
comment:38 Changed 5 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Also "Programming Language :: Python :: 2.4" in setup.py