Opened 3 years ago

Closed 3 years ago

#5451 task closed fixed (fixed)

deprecate Accessor and related classes in twisted.python.reflect

Reported by: jesstess Owned by: jesstess
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, moijes12 Branch: branches/deprecate-accessor-5451
(diff, github, buildbot, log)
Author: moijes12 Launchpad Bug:

Description

Specifically: Settable, AccessorType, PropertyAccessor, Accessor, and Summer.

They are old and unused. Accessor is only for pre-Python 2.2 code.

Attachments (4)

patch_demo.patch (2.6 KB) - added by moijes12 3 years ago.
This is a demo patch. I'm having some trouble deprecating PropertyAccessorType and Accessor.
patch_5451.patch (5.1 KB) - added by moijes12 3 years ago.
Settable, AccessorType, PropertyAccessor, Accessor, OriginalAccessor and Summer are deprecated ( version used is 12.1.0) . Unit Tests are created in test_reflect.py and news files entry is made in twisted/topfiles
patch_5451.2.patch (5.1 KB) - added by moijes12 3 years ago.
Please use this patch and not the previous one for review.
Twisted_5451.patch (6.5 KB) - added by moijes12 3 years ago.
Required changes are mage

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by moijes12

Hi, I'm having a problem here.


I have deprecated the metaclass twisted.python.reflect.AccessorType and the class twisted.python.reflect.PropertyAccessor. I was able to write the unite test for Accessor but with PropertyAccessor,I am facing a problem.The line of code in the function that causes the problem is propertyAccessor = reflect.AccessorType(,(PropertyAccessorTest(),),{}). It gives error function() argument 1 must be code, not str. Please can someone suggest a way out.

Changed 3 years ago by moijes12

This is a demo patch. I'm having some trouble deprecating PropertyAccessorType and Accessor.

comment:2 Changed 3 years ago by moijes12

  • Keywords review added

comment:3 Changed 3 years ago by moijes12

Hi. I've sent in a demo patch. I'm not sure if this violates any policy but if it does I apologize for it. Please provide some advice on how to deprecate PropertyAccessorType and Accessor especially since they are all related with AccessorType being the metaclass and Summer being the child class of Accessor.

comment:4 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to moijes12

Thanks for your work on this!

The patch doesn't appear to demonstrate any problem, so I don't know if I can really answer your question. I can make a guess, that when you use the deprecated decorator on the definition of Accessor, problems arise - and attempting this, I see the error you mentioned, so I probably guessed right. It's better if you provide a patch (or otherwise complete, self-contained example) that demonstrates the problem you encountered though, so no guessing is required. :)

As for the problem, it's caused by the fact that deprecated is not a valid class decorator. It is only a function decorator. It always returns a function, and so the Summer subclass fails, since you cannot subclass a function.

Try using twisted.python.deprecate.deprecatedModuleAttribute for all of these deprecations, instead. It works on any attribute (including global variables defined using =) and also works on all versions of Python supported by Twisted (class decorators are not supported by Python 2.5, which Twisted still supports).

Changed 3 years ago by moijes12

Settable, AccessorType, PropertyAccessor, Accessor, OriginalAccessor and Summer are deprecated ( version used is 12.1.0) . Unit Tests are created in test_reflect.py and news files entry is made in twisted/topfiles

comment:5 Changed 3 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

Changed 3 years ago by moijes12

Please use this patch and not the previous one for review.

comment:6 Changed 3 years ago by jesstess

  • Owner set to jesstess

comment:7 Changed 3 years ago by jesstess

  • Author set to jesstess
  • Branch set to branches/deprecate-accessor-5451

(In [33556]) Branching to 'deprecate-accessor-5451'

comment:8 Changed 3 years ago by jesstess

  • Author changed from jesstess to moijes12
  • Cc moijes12 added
  • Keywords review removed
  • Owner changed from jesstess to moijes12

Thanks for working on this, moijes12! Your changes look good. A few comments:

  • In all cases, deprecatedModuleAttribute should say why the class is getting deprecated. You could say something about how Accessor is for Python 2.1 and earlier, which we no longer support, and that these classes are old and untested.
  • Please wrap the deprecatedModuleAttribute lines after 79 characters, as described in the coding standard.
  • While exercising these deprecated classes in test_reflect.py, you make some unused assignments that pyflakes warns about. Can you remove the assignments and avoid these warning? :
$ pyflakes twisted/test/test_reflect.py
twisted/test/test_reflect.py:789: local variable 'settable' is assigned to but never used
twisted/test/test_reflect.py:798: local variable 'accessorType' is assigned to but never used
twisted/test/test_reflect.py:807: local variable 'accessorType' is assigned to but never used
twisted/test/test_reflect.py:816: local variable 'accessorType' is assigned to but never used
twisted/test/test_reflect.py:825: 'OriginalAccessor' imported but unused
twisted/test/test_reflect.py:834: local variable 'accessorType' is assigned to but never used

You might find a helper method like lookForDeprecationWarning in twisted/internet/test/test_gtkreactor.py helpful for factoring out the duplicate code in these tests.

I created a branch for this ticket but didn't end up committing anything to it, so please submit a revised patch against trunk.

Thanks again!

Changed 3 years ago by moijes12

Required changes are mage

comment:9 Changed 3 years ago by moijes12

  • Keywords review added

Required changes have been made.

comment:10 Changed 3 years ago by jesstess

  • Owner changed from moijes12 to jesstess

comment:11 Changed 3 years ago by jesstess

(In [33558]) Apply patch Twisted_5451.patch by moijes12 with some cosmetic changes.

refs #5451

comment:12 Changed 3 years ago by jesstess

  • Keywords review removed

Thanks for the revised patch, moijes12. I committed your patch to the branch with a couple of mostly cosmetic changes:

  • AccessorType and friends aren't only for Python 2.2, they are for Python 2.2 and up (they are new-style classes), so I adjusted the deprecation message to reflect that.
  • I removed some trailing whitespace.
  • I'm not sure what text editor you are using, but the way you were wrapping lines isn't the way that emacs or the rest of the file wraps lines. For example, your patch had:
+deprecatedModuleAttribute(Version("Twisted", 12, 1, 0),
+"OriginalAccessor is a reference to class twisted.python.reflect.Accessor "
+"which is deprecated.", "twisted.python.reflect", "OriginalAccessor")

with the continued lines flush with the first line, instead of indented, e.g.:

+deprecatedModuleAttribute(
+    Version("Twisted", 12, 1, 0),
+    "OriginalAccessor is a reference to class twisted.python.reflect.Accessor "
+    "which is deprecated.", "twisted.python.reflect", "OriginalAccessor")

Other than that, build results look good, so I'll go ahead and merge!

comment:13 Changed 3 years ago by jesstess

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

(In [33559]) Merge deprecate-accessor-5451

Author: moijes12
Reviewer: jesstess
Fixes: #5451

Deprecate twisted.python.reflect's Settable, AccessorType,
PropertyAccessor, Accessor, OriginalAccessor and Summer
classes. Accessor is Python 2.1 and earlier only, and all are unused
and untested.

Note: See TracTickets for help on using tickets.