Opened 3 years ago

Closed 3 years ago

#5385 enhancement closed fixed (fixed)

Get rid of references and code specific to Python 2.2

Reported by: jesstess Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords: easy
Cc: jesstess, moijes12, thijs Branch: branches/python22-5385-4
(diff, github, buildbot, log)
Author: exarkun, moijes12, jesstess Launchpad Bug:

Description

Some of these comments can just get removed. Some might require removal of code as well.

twisted/internet/error.py:        # only works in 2.2
twisted/python/dist.py:    Python 2.2's distutils. Pretty similar arguments to getDataFiles,
twisted/python/filepath.py:    # new in 2.2.0
twisted/python/reflect.py:    This is for Python 2.2 and up.
twisted/python/reflect.py:    This is a 2.2-only alternative to the Accessor mixin - just set in your
twisted/python/reflect.py:    """A mixin class for Python 2.2 that uses AccessorType.
twisted/python/reflect.py:    This provides compatability with the pre-2.2 Accessor mixin, up
twisted/test/test_compat.py:        # without replacing isinstance on 2.2 as well :(
doc/core/development/policy/coding-standard.xhtml:    in Python 2.2 to unify classes and types. All classes added to Twisted
doc/core/howto/pb-copyable.xhtml:that were introduced in Python 2.2 could be useful too. The result might be
doc/web/examples/xmlrpc.py:# This module is standard in Python 2.2, otherwise get it from
doc/web/howto/xmlrpc.xhtml:included with Python 2.2 and later.</p>

Attachments (6)

5385-patch.patch (7.1 KB) - added by moijes12 3 years ago.
Modified 1. twisted.internet.error.py : Removed comment 'works only in 2.2' from function getConnectionError 2. twisted.python.filepath.py : Removed comment 'new in 2.2.0' 3. twisted.test.test_compat.py : Modified method testIsinstance of class CompatTestCase.Removed # I'm pretty sure it's impossible to implement this # without replacing isinstance on 2.2 as well :( # self.assert_(isinstance({}, dict)) 4. doc.web.examples : Removed comment # This module is standard in Python 2.2, otherwise get it from # http://www.pythonware.com/products/xmlrpc/ 5. twisted.python.reflect.py : Removed comment 'This provides compatability with the pre-2.2 Accessor mixin, up to a point.' Removed Comment ', or using non-2.1 compatible code' Removed Comment 'This implementation is for Python 2.1.' 6. twisted.python.dist.py : removed comment 'This is necessary for Python 2.2's distutils.' from docstring for function getPackages 7. doc/core/development/policy/coding-standard.xhtml : Removed section on new style classes. 8. doc/core/howto/pb-copyable.xhtml : Removed 'The semi-magical <q>property attributes</q> that were introduced in Python 2.2 could be useful too. The result might be hard to maintain or extend, though.' 9. doc/web/howto/xmlrpc.xhtml : Removed 'that is included with Python 2.2 and later.'
patch_5385.patch (18.8 KB) - added by moijes12 3 years ago.
5385_2.patch (6.1 KB) - added by moijes12 3 years ago.
5385_3.patch (6.5 KB) - added by moijes12 3 years ago.
5385_4.patch (5.5 KB) - added by moijes12 3 years ago.
Attaching patch with taking into consideration exarkun's comments.
patch_5385.2.patch (19.2 KB) - added by moijes12 3 years ago.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 3 years ago by exarkun

  • Keywords easy added

Changed 3 years ago by moijes12

Modified 1. twisted.internet.error.py : Removed comment 'works only in 2.2' from function getConnectionError 2. twisted.python.filepath.py : Removed comment 'new in 2.2.0' 3. twisted.test.test_compat.py : Modified method testIsinstance of class CompatTestCase.Removed # I'm pretty sure it's impossible to implement this # without replacing isinstance on 2.2 as well :( # self.assert_(isinstance({}, dict)) 4. doc.web.examples : Removed comment # This module is standard in Python 2.2, otherwise get it from # http://www.pythonware.com/products/xmlrpc/ 5. twisted.python.reflect.py : Removed comment 'This provides compatability with the pre-2.2 Accessor mixin, up to a point.' Removed Comment ', or using non-2.1 compatible code' Removed Comment 'This implementation is for Python 2.1.' 6. twisted.python.dist.py : removed comment 'This is necessary for Python 2.2's distutils.' from docstring for function getPackages 7. doc/core/development/policy/coding-standard.xhtml : Removed section on new style classes. 8. doc/core/howto/pb-copyable.xhtml : Removed 'The semi-magical <q>property attributes</q> that were introduced in Python 2.2 could be useful too. The result might be hard to maintain or extend, though.' 9. doc/web/howto/xmlrpc.xhtml : Removed 'that is included with Python 2.2 and later.'

comment:2 Changed 3 years ago by moijes12

  • Keywords review added

Modified

  1. twisted.internet.error.py : Removed comment 'works only in 2.2' from function getConnectionError
  2. twisted.python.filepath.py : Removed comment 'new in 2.2.0'
  3. twisted.test.test_compat.py : Modified method testIsinstance of class CompatTestCase.Removed

# I'm pretty sure it's impossible to implement this
# without replacing isinstance on 2.2 as well :(
# self.assert_(isinstance({}, dict))

  1. doc.web.examples : Removed comment

# This module is standard in Python 2.2, otherwise get it from
# http://www.pythonware.com/products/xmlrpc/

  1. twisted.python.reflect.py : Removed comment 'This provides compatability with the pre-2.2 Accessor mixin, up

to a point.'
Removed Comment ', or using non-2.1 compatible code'
Removed Comment 'This implementation is for Python 2.1.'

  1. twisted.python.dist.py : removed comment 'This is necessary for Python 2.2's distutils.' from docstring for function getPackages
  2. doc/core/development/policy/coding-standard.xhtml : Removed section on new style classes.
  3. doc/core/howto/pb-copyable.xhtml : Removed 'The semi-magical <q>property attributes</q> that were introduced in Python 2.2 could be useful too. The result might be hard to maintain or extend, though.'
  4. doc/web/howto/xmlrpc.xhtml : Removed 'that is included with Python 2.2 and later.'

comment:3 Changed 3 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/python22-5385

(In [33429]) Branching to 'python22-5385'

comment:4 Changed 3 years ago by jesstess

  • Branch changed from branches/python22-5385 to branches/python22-5385-2

(In [33430]) Branching to 'python22-5385-2'

comment:5 Changed 3 years ago by jesstess

  • Branch changed from branches/python22-5385-2 to branches/python22-5385-3

(In [33431]) Branching to 'python22-5385-3'

comment:6 Changed 3 years ago by jesstess

(In [33432]) Apply 5385-patch.patch by moijes12.

refs #5385

comment:7 Changed 3 years ago by jesstess

  • Cc moijes12 added
  • Keywords review removed
  • Owner set to moijes12

Thanks for working on this, moijes12! A couple of comments on your patch:

  • Your patch had some trailing whitespace. Please check for and remove it in future patches.
  • The assert removed in twisted/test/test_compat.py can be uncommented instead of removed -- now that we don't have to worry about pre-Python 2.2 that test will pass.
  • These old-style classes in twisted/python/reflect.py are a bit of a mess and should have been deprecated long ago. In lieu of removing the Python 2.1 comment in Accessor (which still kind of matters since we don't want people using it), I've opened ticket #5451 to deprecate all of these old and unused classes.
  • The comment you removed in twisted/internet/error.py suggests that the else clause can go away.
  • the section on new-style classes in doc/core/development/policy/coding-standard.xhtml needs to stay, as that's still a policy we care about.

Can you submit a new patch with these changes?

Thanks again for working on this!

comment:8 Changed 3 years ago by moijes12

Sure! I'll do it.

Also, as for the old classes, I had written about it in the mail list. I am in favor of removing Accessor and its related classes. I'll work on that ticket too. I'd also suggest linking #5451 to this ticket or marking it as a duplicate of this ticket as the title of this ticket is "Get rid of references and code specific to Python 2.2". I'll be working on it.

Changed 3 years ago by moijes12

comment:9 Changed 3 years ago by moijes12

  • Keywords review added

Hi. I have attached the latest patch. This should also fix #5451. Below are the details of the patch :
[BR]

  • twisted.internet.error.py - Removed if part and kept only else part. If part was only for Python 2.2.
  • twisted.python.filepath.py - Removed comment 'new in 2.2.0'
  • twisted.test.test_compat.py - Kept only the assert from method testIsInstance of CompatTestCase
  • doc.web.examples - Removed Python 2.2 specific lines.
  • twisted.python.reflect.py -
    • Removed classes AccessorType, PropertyAccessor, Accessor, Summer. Changed all in reflect.py so that it doesn't contain the names of the removed classes.
    • Copied reallySet from Summer(which was removed) to QueueMethod. Modified name and calls in QueueMethod.init so that they contain some default values.
  • twisted.test.test_reflect.py -
    • Removed classes AccessorTester, PropertyAccessorTester, AccessorTest, PropertyAccessorTest
    • Changed test_namedClassLookup to use an object of t.python.reflect.QueueMethod instead of t.python.reflect.Summer. Identical change made to method test_namedAnyClassLookup of the same class.
    • Changed test_namedAnyAttributeLookup to use QueueMethod.reallySet instead of Summer.reallySet. Similarly changed test_namedAnySecondaryLookup.
    • Changed test_attributeExceptions to use QueueMethod instead of Summer to test behavior of ImportError in scenarion import existant.existant.nonexistant .
    • Changed test_boundMethod and test_unboundMethod of class FullyQualifiedNameTests to use QueueMethod().reallySet and QueueMethod.reallySet instead of PropertyAccessor().reallyDel and PropertyAccessor.reallyDel.
    • Changed test_class in class FullyQualifiedNameTests to use reflect.QueueMethod.
  • twisted.python.dist.py - removed comment 'This is necessary for Python 2.2's distutils.' from docstring for function getPackages
  • doc/core/howto/pb-copyable.xhtml : Removed 'The semi-magical <q>property attributes</q> that were introduced in Python 2.2 could be useful too. The result might be hard to maintain or extend, though.'
  • doc/web/howto/xmlrpc.xhtml : Removed 'that is included with Python 2.2 and later.'
  • doc/core/development/policy/coding-standard.xhtml - Left untouched as suggested.

comment:10 Changed 3 years ago by thijs

  • Cc thijs added
  • Keywords review removed

Thanks!

There are nested_scopes future imports for Python >= 2.1 that should go:

./twisted/words/im/pbsupport.py:from __future__ import nested_scopes
./twisted/protocols/htb.py:from __future__ import nested_scopes
./twisted/test/test_defgen.py:from __future__ import generators, nested_scopes
./twisted/persisted/journal/base.py:from __future__ import nested_scopes
./twisted/conch/scripts/tkconch.py:from __future__ import nested_scopes
./twisted/spread/ui/gtk2util.py:from __future__ import nested_scopes
./doc/core/examples/pbgtk2.py:from __future__ import nested_scopes
./doc/web/examples/lj.rpy.py:from __future__ import nested_scopes

There are also future imports for generators that also seem Python 2.2 specific:

./twisted/protocols/ident.py:from __future__ import generators
./twisted/test/test_defgen.py:from __future__ import generators, nested_scopes
./twisted/scripts/tkunzip.py:from __future__ import generators
./twisted/internet/_threadedselect.py:from __future__ import generators
./twisted/trial/test/detests.py:from __future__ import generators
./doc/core/examples/threadedselect/pygamedemo.py:from __future__ import generators
./doc/core/examples/longex2.py:from __future__ import generators

And there 's a from __future__ import division statement, part of Python 2.2:

./twisted/python/log.py:from __future__ import division

ps. make sure to read the WikiFormatting page to prevent your comment markup of getting screwed up

Changed 3 years ago by moijes12

comment:11 Changed 3 years ago by moijes12

Please ignore the patch 5385_2.patch

Changed 3 years ago by moijes12

comment:12 Changed 3 years ago by moijes12

  • Keywords review added

comment:13 Changed 3 years ago by moijes12

The entire suite of tests has been run. I can't see any failures. Please check once if you can.

comment:14 Changed 3 years ago by exarkun

And there 's a from future import division statement, part of Python 2.2:

__future__.division may have been added in Python 2.2, but that's the incorrect criterion to apply for determining when uses of it can be removed. The behavior enabled by __future__.division is not the default in any version of Python 2.x. __future__.division imports must remain.

Changed 3 years ago by moijes12

Attaching patch with taking into consideration exarkun's comments.

comment:15 Changed 3 years ago by moijes12

Please use the patches named below for review

  • patch_5385.patch
  • 5385_4.patch

comment:16 Changed 3 years ago by exarkun

  • Keywords review removed

Though there is a branch linked to this ticket, the mentioned patches appear to be against trunk. I reviewed them against trunk and ignored the branch. Dealing with two patch files doesn't seem to offer much utility, so if possible, in the future, please put all changes into a single patch file. Thanks!

  1. The mention of properties in doc/core/howto/pb-copyable.xhtml is as relevant now as when it was written. Perhaps we don't need to point out that properties were introduced in Python 2.2 or call them "semi-magical" though.
  2. The "new in 2.2.0" in twisted/python/filepath.py is talking about Twisted 2.2.0. However, there's still no harm in deleting it, as it is probably misleading (it's not clear what extent it has: it's probably not supposed to be talking about __cmp__, as it was added directly above children and walk in r14920).
  3. All of the deletions in twisted/python/reflect.py are wrong. See CompatibilityPolicy.
  4. The change to twisted/python/dist.py only makes the code harder to understand. If the entire function is only necessary for Python 2.2, then the proper change would be to get rid of the function and its uses, not to just delete the documentation explaining it is only necessary for Python 2.2.
  5. It's not really obvious to me that the change in twisted/internet/error.py is correct. I suspect this code is poorly tested and the change is wrong. Preserving the socket.gaierror handling is probably the correct thing to do. The Python 2.2 reference is about how pre-Python 2.2 socket modules do not have socket.gaierror. This change needs unit tests, in any case.
  6. The uncommented assertion in twisted/test/test_compat.py isn't adding any value. The name dict is resolved as a builtin. We don't need a test to prove that {} is an instance of the builtin dict. The meaningful test would be that {} is an instance of our dict, which we don't even have anymore. In fact, that whole test method looks useless and should probably be considered for deletion. At the very least, it should use assertTrue, be named according to the naming convention, and have a docstring.
  7. The tests in twisted/test/test_reflect.py should of course remain.
  8. The change will need a news fragment, probably a .misc, however as these don't survive being put in patch files very well you can probably ignore this.
  9. All the modules that are changed by just having a generators or nested_scopes future import removed could also have their module docstring updated to the proper modern formatting proscribed by the coding standard.

Thanks again!

comment:17 Changed 3 years ago by thijs

(In [33523]) apply 5385_4.patch, refs #5385

comment:18 Changed 3 years ago by moijes12

I'd suggest we hold on with this ticket till we close #5451.

comment:19 Changed 3 years ago by itamar

#5451 has been closed.

comment:20 follow-up: Changed 3 years ago by moijes12

Hi. Please, please delete the branch. I'll resubmit everything against the trunk.

comment:21 in reply to: ↑ 20 Changed 3 years ago by thijs

  • Author changed from jesstess to moijes12, jesstess

Replying to moijes12:

Hi. Please, please delete the branch. I'll resubmit everything against the trunk.

Don't worry about the branch, we can simply make a python22-5385-4 branch based on the trunk and apply the patch there.

Changed 3 years ago by moijes12

comment:22 Changed 3 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted
  1. doc/core/howto/pb-copyable.xhtml left untouched.
  2. Points 3 and 7 mentioned by exarkun in his last comment have been taken care-of in #5451.
  3. For Point 8, I've put in a top file.
  4. For point-9, All changes have docstrings( I couldn't file how to add module docstrings). Any guidance would be appreciated.
  5. Point 4 and 5, code left untouched. The code isn't Python 2.2 specific ( atleast in case of point 4).
  6. Point 6, I have removed the 1st and 3rd check. Also, docstrings is updated and assertTrue is used.


Patch is built against trunk. Hope this suffices. But I'll work nevertheless.

comment:23 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks for your continued efforts on this ticket, moijes12.

  1. The strings you added explaining why __future__.generators is not being used aren't necessary. There's no need to explain why a feature isn't being used (there are a lot of Python language features not used in these examples, imagine if the reasons for not using all of them were explained :) Sorry if my earlier comment about updating docstrings confused you. I wasn't asking for this, I was asking for simple formatting fixes to those docstrings.
  2. misc news fragment files should be empty (their contents are ignored)

It'd be great to see this ticket closed, so I think I'll go ahead and remove the docstring additions and then merge the changes. Thanks.

comment:24 Changed 3 years ago by exarkun

  • Author changed from moijes12, jesstess to exarkun, moijes12, jesstess
  • Branch changed from branches/python22-5385-3 to branches/python22-5385-4

(In [34465]) Branching to 'python22-5385-4'

comment:25 Changed 3 years ago by exarkun

(In [34466]) Apply patch_5385.2.patch

refs #5385

comment:26 Changed 3 years ago by exarkun

(In [34467]) And the unadded news fragment

refs #5385

comment:27 Changed 3 years ago by exarkun

(In [34468]) Get rid of unnecessary explanations; some other misc docstring and header fixes

refs #5385

comment:28 Changed 3 years ago by exarkun

(In [34475]) Get rid of this test entirely, there is no longer any associated code and it.

refs #5385

comment:29 Changed 3 years ago by exarkun

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

(In [34477]) Merge python22-5385-4

Author: moijes12
Reviewer: exarkun
Fixes: #5385

Remove some Python 2.2 support code which is obviously no longer
necessary.

Note: See TracTickets for help on using tickets.