Opened 11 years ago
Closed 9 years ago
#5386 enhancement closed fixed (fixed)
Get rid of references and code specific to Python 2.3
Reported by: | jesstess | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | core | Keywords: | |
Cc: | jesstess, Thijs Triemstra | Branch: |
branches/python-2.3-5386-4
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs, rockstar |
Description
Some of these comments can just get removed. Some might require removal of code as well.
twisted/conch/test/test_cftp.py: # Python 2.3 compatibility fix twisted/lore/lint.py:# parser.suite in Python 2.3 raises SyntaxError, <2.3 raises parser.ParserError twisted/python/_release.py: "Version should be in a form kind of like '1.2.3[pre4]'") twisted/python/components.py: warnings.warn("components.backwardsCompatImplements doesn't do anything in Twisted 2.3, stop calling it.", ComponentsDeprecationWarning, stacklevel=2) twisted/python/components.py: warnings.warn("components.fixClassImplements doesn't do anything in Twisted 2.3, stop calling it.", ComponentsDeprecationWarning, stacklevel=2) twisted/python/reflect.py: execName == importName): # python 2.3, no cleanup twisted/python/reflect.py: # Necessary for cleaning up modules in 2.3. twisted/python/syslog.py:# These defaults come from the Python 2.3 syslog docs. twisted/python/util.py: return a negative value. Python 2.3 shares this behavior, but also twisted/python/util.py: To use this function safely you must use the return value. In Python 2.3, twisted/spread/jelly.py:Python 2.3; this can be accomplished by using L{twisted.python.compat.set}. twisted/spread/jelly.py: Python 2.3), and 'module' types, as well as basic types. twisted/test/test_reflect.py: # Make sure that this behavior is *consistent* for 2.3, where there is twisted/test/test_reflect.py: Test references search through a deque object. Only for Python > 2.3. twisted/trial/runner.py: differences between unittest in Python 2.3, 2.4, and 2.5. These twisted/trial/runner.py: In Python 2.4, doctests have correct id() behaviour. In Python 2.3, twisted/trial/test/test_doctest.py: doctest in Python 2.3. twisted/trial/test/test_doctest.py: # doctest reports failures as errors in 2.3 twisted/trial/test/test_loader.py: "Python 2.3 import semantics make this behavior incorrect on that " twisted/trial/test/test_loader.py: "was imported will succeed on Python 2.3, whereas it will fail on " twisted/trial/unittest.py: # we implement this because Python 2.3 unittest defines this code twisted/trial/unittest.py:# Support for Python 2.3 twisted/trial/unittest.py: # Python 2.3's TestSuite doesn't support iteration. Let's monkey patch it! twisted/web/http_headers.py: # Python 2.3 DictMixin.setdefault is defined so as not to have a default twisted/web/http_headers.py: # like dict.setdefault on Python 2.3. -exarkun twisted/web/test/test_http_headers.py: "Python 2.3 does not support keyword arguments to dict.update.") twisted/words/protocols/jabber/sasl.py:# Python 2.4. For Python 2.3 compatibility, the legacy interface is used while twisted/words/protocols/jabber/xmpp_stringprep.py: "recommended you upgrade to Python 2.3.2 or newer if you " doc/core/development/policy/coding-standard.xhtml: example, Python 2.3's <code>sets</code> module was deprecated in Python 2.6 doc/core/development/policy/test-standard.xhtml:folder. This option requires Python 2.3.3 or newer.</p>
Attachments (1)
Change History (46)
comment:1 Changed 11 years ago by
Keywords: | easy added |
---|
Changed 10 years ago by
Attachment: | 5386.patch added |
---|
comment:2 Changed 10 years ago by
Keywords: | review added |
---|
comment:3 Changed 10 years ago by
Owner: | set to jesstess |
---|
comment:4 Changed 10 years ago by
Author: | → jesstess |
---|---|
Branch: | → branches/python-2.3-5386 |
(In [33790]) Branching to 'python-2.3-5386'
comment:6 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from jesstess to rockstar |
Thanks for working on this, rockstar! Some comments:
twisted/python/util.py
- In
mergeFunctionMetadata
,merged
always needs to be set tog
now instead of only in theelse
block, ormerged
won't be defined if aTypeError
is raised. merged.__dict__.update(g.__dict__)
seems like a copy-paste error sincemerged
is already based ong
, and commenting it out doesn't result in any test failures.- The code removals result in some unused imports that can be deleted:
$ pyflakes twisted/python/util.py twisted/python/util.py:5: 'inspect' imported but unused twisted/python/util.py:6: 'types' imported but unused
twisted/spread/jelly.py
- Can you linewrap the textual changes to less than 80 characters (as described in the coding standard).
twisted/test/test_reflect.py
- The way I read this test is that the second
assertRaises
intest_importExceptions
is for confirming thatreflect.namedAny
will propagate Exceptions correctly in Python 2.3 even though there isn't post-failed-import cleanup. http://www.python.org/getit/releases/2.4/highlights/ seems to back this up. Do you agree? If you do, the secondassertRaises
can go away with the comment. test_deque
is for Python > 2.3, so it should stay, but the comment about being for > 2.3 can go away.
twisted/trial/runner.py
- DocTestCase is referenced in several places still. It would be cleanest to take care of removing it and its references in a separate removal ticket, where it gets its own NEWS file. Can you undo these deletions for now and open a removal ticket?
There are some lingering references to Python 2.3 that can be removed:
- twisted/web/test/test_http_headers.py: "Python 2.3 does not support keyword arguments to dict.update.")
- twisted/spread/jelly.py: Python 2.3), and 'module' types, as well as basic types.
Please also add a NEWS file as described in http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles.
Please make future patches against the branch for this ticket.
Here are the build results so far.
Thanks!
comment:7 Changed 10 years ago by
Keywords: | review added |
---|
Ugh. Sorry for the kamikaze contribution. I was more tired than I thought the other night. :)
I've made the requested changes directly into the branch you created. The Build results are here.
I've also filed ticket #5554 after re-instating DocTestCase it twisted.trial.runner
comment:8 Changed 10 years ago by
Owner: | rockstar deleted |
---|
comment:9 Changed 10 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:10 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to rockstar |
Status: | assigned → new |
Some more feedback, mostly about extra things I think you can remove, but also about a few things I think we need to be careful to preserve:
- twisted/test/test_reflect.py
- The deleted assertion about
ZeroDivisionError
being raised bynamedAny("twisted.test.reflect_helper_ZDE")
should be restored. It's great that the implementation for that behavior is now simpler, but we still need to verify the behavior is provided. test_deque
is mis-indented, so it won't ever be run- Also,
collections.deque
was added in Python 2.4, so it should always import. The module doesn't need to protect its import with anexcept ImportError
, nor skip the test if it can't be imported.
- The deleted assertion about
- twisted/trial/unittest.py
- I'm a bit worried about the side-effects of deleting
TestSuite.run
. However, all our tests pass, so I guess I shouldn't be. It might be nice if you could convince jml or lifeless (irc nicks) to comment on this part of the patch, though. - The line
iter(pyunit.TestSuite())
is now sitting in the module all by itself. It has no side-effects, it should be deleted.
- I'm a bit worried about the side-effects of deleting
- twisted/python/util.py
- The merged name is actually pointless now. The function can just use g throughout.
- This also means that to use the function safely, it is no longer necessary to use the return value. It is the same as the function object passed in as an argument. So, the docstring does not need to claim that the return value must be used anymore.
- twisted/spread/jelly.py
- I think that, in
allowInstancesOf
, dropping Python 2.3 support means that the'class'
value being passed toallowTypes
is superfluous. There will only be a builtin "classobj" type to allow, never a builtin "class" type. If this is right, we should get rid of the'class'
parameter. We should make sure we have good unit test coverage for these cases though (not just delete it and hope).
- I think that, in
- twisted/lore/lint.py
- I think you should leave
parserErrors
defined, but define it as just(SyntaxError,)
. It's a public name, even if it's a terrible one. We don't want to break any code that was importing this.
- I think you should leave
- twisted/words/protocols/jabber/xmpp_stringprep.py
- The entire code block beneath
if sys.version_info < (2,3,2):
can go, can't it? - This seems to imply that some of the later code guarded by the
if crippled:
check can go as well.
- The entire code block beneath
- twisted/trial/test/test_doctest.py
- I'd lean towards keeping
test_id
which is deleted in the branch. PerhapsDocTest
behaves fine in all our currently supported Python versions, but keeping the check will let us know if that continues to be the case going forward. - The other change in this file, to just compare against the length of
failures
, is cool. Specification tightening. :)
- I'd lean towards keeping
- The news fragment in topfiles/ doesn't follow the style guide. I might go for a .misc here anyway, for ease. However, if you like, you could make a news fragment for _each_ subproject and mention the main area of code cleanup for each subproject in each of those files.
Thanks!
comment:11 Changed 10 years ago by
Oh, one more thing, there's a doc error now: http://buildbot.twistedmatrix.com/builders/documentation/builds/1954/steps/api-documentation/logs/stdio
comment:12 Changed 10 years ago by
I saw r33901 that removes the Python <2.3.2 stuff for XMPP stringprep. As we are also dropping 2.4 support, you might also want to address the try/except
import for unicodedata in this patch, as this block is being dedented now.
comment:13 follow-up: 14 Changed 10 years ago by
Presumably that change should be addressed in the patch for the ticket that drops Python 2.4 support.
comment:14 Changed 10 years ago by
Replying to exarkun:
Presumably that change should be addressed in the patch for the ticket that drops Python 2.4 support.
Of course, but that would surely create a merge conflict in both directions. That's why I suggested it here.
comment:15 Changed 10 years ago by
Yea. There's going to be merge conflicts. Probably more than a few.
comment:16 Changed 10 years ago by
Yeah, I've already run into a few of them already, which is why I haven't re-submitted for review just yet. I expect that things will mellow out in a few days and I'll be able to get this patch completed.
comment:17 Changed 10 years ago by
Cc: | Thijs Triemstra added |
---|
comment:18 Changed 9 years ago by
Author: | jesstess → thijs, jesstess |
---|---|
Branch: | branches/python-2.3-5386 → branches/python-2.3-5386-2 |
(In [36883]) Branching to 'python-2.3-5386-2'
comment:19 Changed 9 years ago by
comment:20 Changed 9 years ago by
Author: | thijs, jesstess → rockstar, thijs |
---|---|
Keywords: | review added |
Owner: | rockstar deleted |
Replying to exarkun:
- twisted/trial/unittest.py
- I'm a bit worried about the side-effects of deleting
TestSuite.run
. However, all our tests pass, so I guess I shouldn't be. It might be nice if you could convince jml or lifeless (irc nicks) to comment on this part of the patch, though.
Removed __call__
.
- The line
iter(pyunit.TestSuite())
is now sitting in the module all by itself. It has no side-effects, it should be deleted.
Fixed.
- twisted/python/util.py
- The merged name is actually pointless now. The function can just use g throughout.
- This also means that to use the function safely, it is no longer necessary to use the return value. It is the same as the function object passed in as an argument. So, the docstring does not need to claim that the return value must be used anymore.
Fixed.
We can also ignore the reference to Python 2.3 for unsignedID
that has been deprecated recently (#5544).
- twisted/spread/jelly.py
- I think that, in
allowInstancesOf
, dropping Python 2.3 support means that the'class'
value being passed toallowTypes
is superfluous. There will only be a builtin "classobj" type to allow, never a builtin "class" type. If this is right, we should get rid of the'class'
parameter. We should make sure we have good unit test coverage for these cases though (not just delete it and hope).
This could be done in a new ticket?
- twisted/lore/lint.py
- I think you should leave
parserErrors
defined, but define it as just(SyntaxError,)
. It's a public name, even if it's a terrible one. We don't want to break any code that was importing this.
Fixed.
- twisted/words/protocols/jabber/xmpp_stringprep.py
- The entire code block beneath
if sys.version_info < (2,3,2):
can go, can't it?- This seems to imply that some of the later code guarded by the
if crippled:
check can go as well.
Fixed.
- twisted/trial/test/test_doctest.py
- I'd lean towards keeping
test_id
which is deleted in the branch. PerhapsDocTest
behaves fine in all our currently supported Python versions, but keeping the check will let us know if that continues to be the case going forward.
Fixed.
- The news fragment in topfiles/ doesn't follow the style guide. I might go for a .misc here anyway, for ease. However, if you like, you could make a news fragment for _each_ subproject and mention the main area of code cleanup for each subproject in each of those files.
Added a .misc file for each project that received changed.
Please review.
comment:21 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:22 follow-up: 24 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Thijs Triemstra |
Status: | assigned → new |
Thanks.
I think that, in allowInstancesOf, dropping Python 2.3 support means that the 'class' value being passed to allowTypes is superfluous.
This could be done in a new ticket?
Sure.
- The second hunk in
twisted/spread/jelly.py
seems like an undesirable removal to me. It's providing potentially interesting information, I don't think it should be removed. - Also, there is code that can be deleted from
twisted/spread/jelly.py
that has been left in place._set
will never be set toNone
on a supported version of Python now; its definition is now redundant (always the same as the builtinset
) and all of the code that is conditional on it not beingNone
no longer needs to make the checks it makes.
I forced a build, hopefully it will be green. Please re-submit once the jelly changes have been made. Thanks again.
comment:23 Changed 9 years ago by
Status: | new → assigned |
---|
The windows build failed with:
[ERROR] Traceback (most recent call last): File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\trial\runner.py", line 562, in loadPackage module = modinfo.load() File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\python\modules.py", line 383, in load return self.pathEntry.pythonPath.moduleLoader(self.name) File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\python\_reflectpy3.py", line 262, in namedAny topLevelPackage = _importAndCheckStack(trialname) File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\python\_reflectpy3.py", line 209, in _importAndCheckStack reraise(excValue, excTraceback) File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\conch\test\test_cftp.py", line 17, in <module> from twisted.conch import unix File "C:\buildslave\twisted\winxp32-py2.7\Twisted\twisted\conch\unix.py", line 17, in <module> import fcntl, tty exceptions.ImportError: No module named fcntl twisted.conch.test.test_cftp
comment:24 Changed 9 years ago by
Keywords: | review added; easy removed |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | assigned → new |
Replying to exarkun:
- The second hunk in
twisted/spread/jelly.py
seems like an undesirable removal to me. It's providing potentially interesting information, I don't think it should be removed.
Fixed.
- Also, there is code that can be deleted from
twisted/spread/jelly.py
that has been left in place._set
will never be set toNone
on a supported version of Python now; its definition is now redundant (always the same as the builtinset
) and all of the code that is conditional on it not beingNone
no longer needs to make the checks it makes.
- Removed the imports and checks, inc the deprecation warning, introduced 5 yrs ago (r22570).
decimal
was introduced in 2.4 so that conditional import and it's code could be removed as well..
Also:
- fixed the import in
test_cftp
andtest_filetransfer
, picked up by the win32 buildslave - restored the
parserErrors
exception inlore.lint
- added a test for the exception handling in
util.mergeFunctionMetadata
. Also silenced theAttributeError
when trying to merge to a read-only__module__
.
Forced a new build.
comment:25 follow-up: 27 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
- I don't think removing the serialization of
sets.Set
andsets.ImmutableSet
is a compatible change. (Removing the *un*serialization is, since that code is never exercised) set
andfrozenset
intwisted.python.compat
are no longer needed. The import logic for them should be removed here, and perhaps a ticket filed to deprecate them.- We don't need to point people at
twisted.python.compat.set
anymore (injelly.py
)
- We don't need to point people at
- I don't think
parserErrors
should be *used* in lore, just defined.- A comment that it is there for compatibility, and perhaps a ticket to deprecate it might be warranted.
Please resubmit after addressing the above comments.
comment:27 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Replying to tom.prince:
- I don't think removing the serialization of
sets.Set
andsets.ImmutableSet
is a compatible change. (Removing the *un*serialization is, since that code is never exercised)
Restored the serialization.
set
andfrozenset
intwisted.python.compat
are no longer needed. The import logic for them should be removed here, and perhaps a ticket filed to deprecate them.
Removed the imports from test_jelly.
- We don't need to point people at
twisted.python.compat.set
anymore (injelly.py
)
Removed reference.
- I don't think
parserErrors
should be *used* in lore, just defined.
Reverted back to SyntaxError
.
Forced a new build.
comment:28 follow-up: 30 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
- Please remove the conditional import of
set
andfrozenset
intwisted.python.compat
(but leave the aliases there). test_oldImmutableSets
andtest_oldSets
have odd indentation. (reported here).
Please address the above issues then commit after a green buildbot run.
comment:30 Changed 9 years ago by
Status: | new → assigned |
---|
Replying to tom.prince:
- Please remove the conditional import of
set
andfrozenset
intwisted.python.compat
(but leave the aliases there).
Done. Opened #6297 to remove it's usage and deprecate it.
test_oldImmutableSets
andtest_oldSets
have odd indentation. (reported here).
Fixed.
comment:31 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:32 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
(In [37129]) Revert r37124: LookupsTestCase.test_importExceptions failing on python 3.3
test_importExceptions is failing on Python 3.3 only because a statement was modified to clean up Python 2.3 code which introduced an issue in test_reflectpy3.LookupsTestCase. This wasn't caught initially because the buildslave for python 3.3 wass down.
====================================================================== FAIL: test_importExceptions (twisted.python.test.test_reflectpy3.LookupsTestCase) test_importExceptions
Traceback (most recent call last):
File "twisted/trial/_synctest.py", line 328, in assertRaises
result = f(*args, kwargs)
File "twisted/python/_reflectpy3.py", line 273, in namedAny
obj = getattr(obj, n)
AttributeError: 'module' object has no attribute 'reflect_helper_IE'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "twisted/trial/_synctest.py", line 1185, in _run
runWithWarningsSuppressed(suppress, method)
File "twisted/python/util.py", line 1065, in runWithWarningsSuppressed
return f(*args, kwargs)
File "twisted/python/test/test_reflectpy3.py", line 290, in test_importExceptions
reflect.namedAny, "twisted.test.reflect_helper_IE")
File "software/twisted/svn/Twisted/trunk/twisted/trial/_synctest.py", line 335, in assertRaises
failure.Failure().getTraceback()))
twisted.trial.unittest.FailTest: <class 'AttributeError'> raised instead of ImportError:
Traceback (most recent call last):
File "twisted/trial/_synctest.py", line 1216, in _runFixturesAndTest
if self._run(suppress, todo, method, result):
File "twisted/trial/_synctest.py", line 1185, in _run
runWithWarningsSuppressed(suppress, method)
File "twisted/python/util.py", line 1065, in runWithWarningsSuppressed
return f(*args, kwargs)
File "twisted/python/test/test_reflectpy3.py", line 290, in test_importExceptions
reflect.namedAny, "twisted.test.reflect_helper_IE")
--- <exception caught here> ---
File "twisted/trial/_synctest.py", line 328, in assertRaises
result = f(*args, kwargs)
File "twisted/python/_reflectpy3.py", line 273, in namedAny
obj = getattr(obj, n)
builtins.AttributeError: 'module' object has no attribute 'reflect_helper_IE'
comment:34 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | reopened → new |
comment:36 Changed 9 years ago by
Replying to tom.prince:
It is now back up, and appears to be green.
Was this a review cause the keyword's still there.. :)
comment:37 follow-up: 38 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
twisted.python.test.util.TestMergeFunctionMetadata.test_moduleBuiltinTypeNotMerged
doesn't seem to make sense. Please file a ticket.twisted/spread/jelly.py
: In the docstring that references python2.3,classobj
should be the primary name, andclass
mentioned parentheically (reversing the current situation).- On further considertion, there is some weirdness here, so I'll file a ticket to deal with it.
- doc/core/howto/debug-with-emacs.xhtml has a reference to 2.3. It probably wants to be replaced with
python -m pdb
doc/core/development/policy/coding-standard.xhtml
referencesset
/frozenset
incompat
. It doesn't appear that there are any modules that currently fall in to that category. Thus, we probably want to keep that example, but mention that it is historical.
Please resubmit for review after addressing 1+3+4.
comment:38 Changed 9 years ago by
Branch: | branches/python-2.3-5386-2 → branches/python-2.3-5386-3 |
---|---|
Keywords: | review added |
Owner: | Thijs Triemstra deleted |
Replying to tom.prince:
twisted.python.test.util.TestMergeFunctionMetadata.test_moduleBuiltinTypeNotMerged
doesn't seem to make sense. Please file a ticket.
Removed the test. It was to catch an uncovered line or 2 in mergeFunctionMetadata but it's not that important for a new ticket i think.
- doc/core/howto/debug-with-emacs.xhtml has a reference to 2.3. It probably wants to be replaced with
python -m pdb
Fixed.
doc/core/development/policy/coding-standard.xhtml
referencesset
/frozenset
incompat
. It doesn't appear that there are any modules that currently fall in to that category. Thus, we probably want to keep that example, but mention that it is historical.
Rewrote that part.
- There were also 2 fallbacks used for
collections.deque
imports that were also removed - the
crippled
attribute in twisted/words/protocols/jabber/xmpp_stringprep.py was public and since there might be people out there using it it was restored and deprecated instead.
Forced a new build.
comment:39 Changed 9 years ago by
Keywords: | review removed |
---|
twisted.lore.lint.parserErrors
should be deprecated.
comment:40 Changed 9 years ago by
- The various deprecated attributes should be mentioned in the appropriate news files.
And, please merge, after doing that (with test).
comment:41 Changed 9 years ago by
Owner: | set to Thijs Triemstra |
---|---|
Status: | new → assigned |
comment:42 Changed 9 years ago by
Author: | rockstar, thijs → thijs, rockstar |
---|---|
Branch: | branches/python-2.3-5386-3 → branches/python-2.3-5386-4 |
(In [37860]) Branching to 'python-2.3-5386-4'
comment:45 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Remove mentions of Python 2.3 and code where applicable.