Opened 5 years ago

Closed 5 years ago

#8124 enhancement closed fixed (fixed)

Update @deprecated to work with properties and __init__

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/deprecated-property-8124
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

from glyph

If you care about inheritance, the implementation is a little bit tricky, because you have to manually walk the class hierarchy looking for the attribute. But conceptually it's pretty simple: just look at the type of 'oself' in the get method of the returned descriptor.

Right now, @deprecated is hard-coded to assume a function, but it could be repurposed to work with specific other descriptor types reasonably easily. Arbitrary descriptors might be hard, because it's not clear when to emit the message, but specific types like @property should be pretty straightforward with an instance check.

Change History (10)

comment:1 Changed 5 years ago by Adi Roiban

Author: adiroiban
Branch: branches/deprecated-property-8124

(In [46325]) Branching to deprecated-property-8124.

comment:2 Changed 5 years ago by Adi Roiban

Summary: Update @deprecated to work with propertiesUpdate @deprecated to work with properties and __init__

comment:3 Changed 5 years ago by Adi Roiban

Keywords: review added
Owner: changed from Adi Roiban to hawkowl

I am lost and Amber said she might be able to help here.

Test are in the branch :)

You can check if the tests are valid and if they show correct usage of @deprecated

If you know how to update the code to have the tests pass please commit to this branch.

Also feel free to update the tests is they don't make sense.

Thanks!

comment:4 Changed 5 years ago by hawkowl

Keywords: review removed

Thanks Adi, the tests were a good starting point -- I've updated it with how @deprecated is used on classes existingly -- as a class decorator, not decorating init.

comment:5 Changed 5 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

I've added some implementations -- I couldn't get it to work with just @deprecated, as it didn't handle the case where you deprecated the *property* -- @wraps just completely falls apart. So, I deprecate the inner method which is a function -- not a method, so the existing FQDN lookup stuff doesn't know it's on a class!, and because we know it's a property, we can stack walk and probably get the right answer.

So yes, please review. Builders are spun.

comment:6 Changed 5 years ago by Adi Roiban

Owner: set to hawkowl

Thanks for your update.

I have updated the test to have all decorated related tests into a single testcase.

I have also updated them to check for the deprecatedVersion attribute which is set by the decorator... but I have no idea why we need it.

I have also updated the test to include a test for the setter decorator.

I then tried an ugly change which will only require a single decorator so we can have something like this

    @deprecatedProperty(Version('Twisted', 1, 2, 3))
    def someProperty(self):
        """
        Getter docstring.
        """
        return self._someProtectedValue


    @someProperty.setter
    def someProperty(self, value):
        """
        Setter docstring.
        """
        self._someProtectedValue = value

Amber, please check my latest commits.

Feel free to revert or improve them. I am not happy with deprecatedFunction code duplication but I got tired of fighting Python build-in magic.

Thanks!

comment:7 Changed 5 years ago by hawkowl

They look fine, but since we've both worked on this, we'll need someone else to review it :)

comment:8 Changed 5 years ago by Glyph

Keywords: review removed
Owner: changed from hawkowl to Adi Roiban

comment:9 Changed 5 years ago by hawkowl

Something I noticed:

https://github.com/twisted/twisted/compare/trunk...deprecated-property-8124#diff-29059ef15a88640b2a2751c2a58a938dR800 says "test_classMethod" but we're not actually testing classmethods, it's actually a class. So rename the test method to "test_class" instead I guess?

After that, and the twistedchecker error fixed up, please land, Adi :)

comment:10 Changed 5 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [47001]) Merge deprecated-property-8124: Make @deprecatedProperty, to make deprecated properties

Author: adiroiban, hawkowl Reviewer: glyph Fixes: #8124

Note: See TracTickets for help on using tickets.