Opened 4 years ago

Last modified 3 months ago

#4804 defect new

twisted.python.deprecate.deprecated gets the name of methods wrong

Reported by: glyph Owned by:
Priority: normal Milestone:
Component: core Keywords: review
Cc: Branch: branches/deprecated-instance-methods-4804
(diff, github, buildbot, log)
Author: cyli Launchpad Bug:

Description

If you deprecate a method x on a class Y in the module z, twisted.python.deprecate.deprecated thinks that the FQPN of the thing that it's decorating is z.x and reports it as such, not reporting the fairly interesting detail of the class Y.

It should get this right.

Attachments (1)

deprecate-methods.patch (5.1 KB) - added by Julian 10 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by glyph

  • Status changed from new to assigned

I'd really like to update the compatibility policy to recommend this decorator instead of ad-hoc warnings.warn calls, so that we can have a systematic approach to deprecation and consistent error messages, but this seems like a blocker for doing that, so I'll work on it this weekend.

comment:2 Changed 4 years ago by exarkun

  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #3254.

comment:3 follow-up: Changed 4 years ago by exarkun

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Okay, perhaps this isn't really a duplicate. Perhaps it's just the way that #3254 should have been resolved in the first place, or something.

Note that #3254 resolved the issue by saying you have to decorate an _unbound method_ instead of the bare function. This is the difference between:

   class Foo:
      def bar(): pass
   Foo.bar = deprecated(Foo.bar)

and

   class Foo:
      @deprecated
      def bar(): pass

Clear the latter is what one might first expect to work.

comment:4 in reply to: ↑ 3 Changed 4 years ago by glyph

  • Status changed from reopened to new

Replying to exarkun:

Okay, perhaps this isn't really a duplicate. Perhaps it's just the way that #3254 should have been resolved in the first place, or something.

Note that #3254 resolved the issue by saying you have to decorate an _unbound method_ instead of the bare function.

Thanks for the clarification.

comment:5 Changed 4 years ago by glyph

  • Status changed from new to assigned

comment:6 Changed 10 months ago by Julian

  • Owner glyph deleted
  • Status changed from assigned to new

#5314 was marked as a duplicate.

comment:7 Changed 10 months ago by exarkun

If the decorator returned a thing that was also a descriptor then I guess the desired behavior could be achieved.

I wonder if there's a library somewhere for writing correct decorators without remembering and re-implementing the the fifty different subtle tricks required...

Changed 10 months ago by Julian

comment:8 Changed 10 months ago by Julian

Here's a possible start.

comment:9 Changed 6 months ago by cyli

  • Author set to cyli
  • Branch set to branches/deprecated-instance-methods-4804

(In [42457]) Branching to deprecated-instance-methods-4804.

comment:10 Changed 6 months ago by cyli

I sincerely apologize. :( I was working on deprecating something that was difficult without this decorator working, so I did some work on this issue on a plane. I should have checked before I got on the plane whether there was a ticket and work outstanding.

Thanks tons for your work on this Julian! I hope it's ok that I uploaded the branch?

This branch passes the python 2.7 and python 3.3 tests locally (I still need to kick off buildbot).

classmethods and staticmethods don't work though. I think classmethods may involve fixing _fullyQualifiedName, but I'm not sure that static methods can work. :|

comment:11 Changed 6 months ago by cyli

  • Keywords review added

comment:12 follow-up: Changed 6 months ago by exarkun

I'd really like to update the compatibility policy to recommend this decorator instead of ad-hoc warnings.warn calls

For what it's worth, we should probably actually recommend deprecatedModuleAttribute. You want a deprecation warning in these cases:

from module import deprecatedThing

import anotherModule
anotherModule.deprecatedThing

but deprecated will not give them to you. I wonder what deprecated *is* good for, actually.

comment:13 in reply to: ↑ 12 Changed 6 months ago by glyph

Replying to exarkun:

I'd really like to update the compatibility policy to recommend this decorator instead of ad-hoc warnings.warn calls

For what it's worth, we should probably actually recommend deprecatedModuleAttribute. You want a deprecation warning in these cases:

from module import deprecatedThing

import anotherModule
anotherModule.deprecatedThing

but deprecated will not give them to you. I wonder what deprecated *is* good for, actually.

Hmm. Good point.

The work we did in this branch (woo pair programming in cramped airplane seats) gets a little closer to @deprecated being good for something. It allows you to deprecate methods, which deprecatedModuleAttribute doesn't let you do. However, it still emits the warning at call time rather than at __get__ time which, as you point out, is probably wrong.

What if we adjusted @deprecated's behavior to always do what deprecatedModuleAttribute does when called at module scope?

comment:14 Changed 6 months ago by exarkun

It allows you to deprecate methods, which deprecatedModuleAttribute doesn't let you do.

Ah yes, thank you for pointing that out.

What if we adjusted @deprecated's behavior to always do what deprecatedModuleAttribute does when called at module scope?

I think that sounds pretty cool. It would be nice if we could say deprecated is the thing to use for anything you can decorate. We'll still need deprecatedModuleAttribute for other stuff but I think classes, functions, and methods are the things we deprecate by far most often.

This is probably all fodder for follow-up tickets, though. The change being proposed here still makes sense as a self-contained improvement.

comment:15 Changed 6 months ago by exarkun

I filed #7217, #7218, and #7219.

comment:16 Changed 6 months ago by Julian

@cyli I certainly don't mind, I'll be happy when this is fixed no matter what, so thanks for pushing this over the edge :D

And +1 on those followups.

comment:17 Changed 3 months ago by danielsank

I wonder if there's a library somewhere for writing correct decorators without remembering and re-implementing the the fifty different subtle tricks required...

I've seen such a library somewhere (1). It was incredibly complex and probably a nightmare to maintain. I much prefer to just have different decorators:

depricated_method
depricated_classmethod

and so on.

(1) http://blog.dscpl.com.au/2014/01/how-you-implemented-your-python.html

Last edited 3 months ago by danielsank (previous) (diff)
Note: See TracTickets for help on using tickets.