Opened 4 years ago

Last modified 3 weeks ago

#4804 defect new

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

Reported by: glyph Owned by: cyli
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/deprecated-instance-methods-4804-2
(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 11 months ago.

Download all attachments as: .zip

Change History (23)

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 11 months ago by Julian

  • Owner glyph deleted
  • Status changed from assigned to new

#5314 was marked as a duplicate.

comment:7 Changed 11 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 11 months ago by Julian

comment:8 Changed 11 months ago by Julian

Here's a possible start.

comment:9 Changed 8 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 8 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 8 months ago by cyli

  • Keywords review added

comment:12 follow-up: Changed 8 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 8 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 8 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 8 months ago by exarkun

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

comment:16 Changed 8 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 4 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 4 months ago by danielsank (previous) (diff)

comment:18 follow-up: Changed 8 weeks ago by rwall

Thanks Julian and cyli,

I haven't finished yet, but here's a partial review...

Notes:

Points:

  1. I see an error when I put @deprecated above @classmethod. AttributeError: 'classmethod' object has no attribute '__module__'. In trunk I see a different error. AttributeError: 'classmethod' object has no attribute '__name__'. However, if @deprecated comes below, the deprecation warning is emitted.
  2. If I supply a reference to a classmethod as the replacement parameter, the message doesn't render the actual FQDN of the replacement. Instead I get the message please use <classmethod object at 0xd84868> instead
  3. twisted/python/deprecate.py
    1. Typo "Actally"
    2. Various missing full-stops after sentences in docstrings and epydoc fields.
    3. _type could be given a more descriptive name...maybe deprecatee_type
    4. Typo "caled"
    5. _DeprecateDescriptor
      1. I think the convention is to document the initialiser parameters and then refer to those in the @ivar epydocs.
  4. twisted/python/test/test_deprecate.py
    1. Various missing full stops.

...more to follow.

comment:19 Changed 3 weeks ago by cyli

  • Branch changed from branches/deprecated-instance-methods-4804 to branches/deprecated-instance-methods-4804-2

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

comment:20 in reply to: ↑ 18 Changed 3 weeks ago by glyph

Replying to rwall:

I think the convention is to document the initialiser parameters and then refer to those in the @ivar epydocs.

There has been some extended and rather diffuse discussion on this topic, so I will try to give some background. Really, this needs to be in the coding standard somewhere and not just repeated on tickets but at least let me try to get the right stuff repeated :-).

When we started requiring epydoc fields everywhere, this resulted in a tremendous amount of duplication of process where people would write a ton of prose for @param in __init__, then get review feedback that they forgot to document an @ivar, and go copy and paste that. So there was a lot of redundancy and the opportunity for things to get de-synchronized.

So we started saying instead that if a value is both a @param and an @ivar, and they have the same name, you only need to document it in one place or the other, because clearly more documentation than that is just busy-work. (Unfortunately this is presented horribly in pydoctor, as the __init__ still shows up as undocumented.) Since a public attribute is a longer-lived aspect of an API than a constructor argument, we generally preferred to document @ivars over @params when choosing one or the other.

This unfortunately lead to some tickets including copious documentation of private @ivar attributes, but no documentation of the public @params for __init__. That's obviously bad, so we started instead saying that you should document things in @param.

So policy has shifted considerably over time, and where we are now is something like:

twistedchecker is going to complain if you don't document every @ivar and every @param, and we want each new commit to be twistedchecker-clean on the buildbot. So you have to document all of them.

aside: I tend to object to workarounds less than exarkun does, but generally speaking workarounds which just make you write more informative documentation that is presented better are less onerous than things which, for example, distort your code to make it less readable to make it pass pyflakes.

Since attributes are more frequently private these days than they used to be, it is good form to document the parameters to __init__ first, since that is the user's entry-point into the API. However, since public attributes are important to document (and because of the aforementioned twistedchecker issue), we now include @ivar descriptions, even if they're redundant; but to make them less redundant, we refer to __init__ and @param.

However, it is possible to actually L{}-link directly to an @ivar, but not possible to L{}-link to a @param as far as I know, so if there are public attributes that are intended to be used externally as attributes and not just constructor arguments then it may make sense to do the reverse and link from the @param to the @ivar.

There should really be a ticket to update the coding standard to lay this out cleanly.

comment:21 Changed 3 weeks ago by exarkun

There has been some extended and rather diffuse discussion on this topic, so I will try to give some background. Really, this needs to be in the coding standard somewhere and not just repeated on tickets but at least let me try to get the right stuff repeated :-).

https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#docstrings

There should really be a ticket to update the coding standard to lay this out cleanly.

We seem to be going the direction of "document every possible thing" these days. This stems from good intentions but the result is ever more bloated developer documentation of which any individual contributor has an ever shrinking knowledge. Rather than continuing to block the docs even further (though we don't have to do so in this particular case since it happened already, hooray) I think we need to get serious about pursuing a different strategy - for example, making twistedchecker a piece of software we could actually maintain and the output of which we could actually rely on (this really is just an example - the notion of a tool that tells you every single thing that's wrong with a piece of software is, of course, quite enticing - but perhaps unachievable).

comment:22 Changed 3 weeks ago by rwall

  • Keywords review removed
  • Owner set to cyli

Thanks Cyli and Glyph

  • Looks like all the comments in my last review have been addressed.
  • The changes look really good, although I'm not sure I understand all the hoops you've had to jump through to support static / class methods and Python3. And the tests look complete.
  • Tests all pass locally on Fedora20

Points:

  1. There's one twistedchecker warning which looks like TC not understanding Python3 private attributes:
  1. I found that when I supply Foo.__str__ as the replacement, the fqdn is just __str__ eg
if __name__ == "__main__":
    import deprecate
    raise SystemExit(deprecate.main())

from twisted.python.deprecate import deprecated
from twisted.python.versions import Version


class Foo(object):
    def to_string(self):
        pass
Foo.to_string = deprecated(Version('Twisted', 14, 12, 1), Foo.__str__)(Foo.to_string)


def main():
    Foo().to_string()

(twisted-4804)[~/projects/Twisted]$ python -W all deprecate.py 
  from twisted.python.deprecate import deprecated
/home/richard/projects/Twisted/deprecate.py:16: DeprecationWarning: deprecate.Foo.to_string was deprecated in Twisted 14.12.1; please use __str__ instead
  Foo().to_string()

  1. I don't see a NEWS file in this second branch.

Please answer or address those points as you see fit and then merge.

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