Opened 6 years ago

Last modified 13 months ago

#3266 enhancement new

Provide tools for managing new deprecation policy

Reported by: jml Owned by:
Priority: normal Milestone:
Component: core Keywords: deprecation policy
Cc: Branch: branches/deprecation-tool-3266-3
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Twisted has recently adopted a new deprecation policy (see thread starting at http://twistedmatrix.com/pipermail/twisted-python/2008-April/017497.html).

We need to have tools to make following this policy as easy as pie.

Change History (19)

comment:1 Changed 6 years ago by exarkun

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

Please re-open if you can describe the tool.

comment:2 Changed 6 years ago by radix

  • Resolution invalid deleted
  • Status changed from closed to reopened

Here is what we should do in the first implementation:

twisted.python.deprecation.deprecate should be a function
twisted.python.deprecation.release_8_1 should be an object which encapsulates both a Version and a Date of release. Either every time a release is done, or every time a deprecation is first added after a particular release, a new release_* constant should be added.

deprecate(most_recent_version, "text", other_warning_arguments) will emit either a PendingDeprecationWarning or a DeprecationWarning, depending on whether the passed in version is more than 6 months and two releases away from whatever the current version is (defined by twisted.version).

FUTURE IDEA:

Provide a twisted.python.deprecation.NEXT_RELEASE which, during release or during an SVN commit hook, will automatically be replaced by the literal name of the constant for the most recent release.

comment:3 Changed 6 years ago by radix

I guess we'll definitely need to associate a date with each new release when it's released, so deprecate() has something to compare the passed version with.

comment:4 Changed 6 years ago by exarkun

  • Owner changed from jml to exarkun
  • Status changed from reopened to new

So, for example, instead of writing:

@twisted.python.deprecate.deprecated(twisted._8_1_0)
def foo(x, y):
    return x + y

we should write:

def foo(x, y):
    deprecate(twisted.python.deprecate.release_8_1_0, "Don't use foo; use bar.", stacklevel=2)

Or should we keep using the current twisted.python.deprecate.deprecated and change the implementation of that function? Perhaps we should still write the first version above, but
twisted.python.deprecate.deprecated should be the thing which is checking dates and knows about the current version number?

The only thing the current API is missing is the date information. We could make that information available (ie, a dict keyed on version objects). On the other hand, maybe that will just result in a fragile mess that we should avoid.

There are four uses of twisted.python.deprecate.deprecated in our code base now and they're all basically the same. They create a new Version giving the version of Twisted they expect to first be released in and then they create a decorator with it.

So okay, I guess we do need a new API. The current one doesn't express enough information for the use-case of Twisted deprecations - it's too general. I'm tempted to put this code in twisted.python._release instead of some other public module anywhere. It's really really really for Twisted only (even more than the release stuff, which just happens to not be general purpose).

comment:5 Changed 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/deprecation-tool-3266

(In [24764]) Branching to 'deprecation-tool-3266'

comment:6 Changed 6 years ago by exarkun

Forget what I said about this being for Twisted only. It clearly has general purpose applications. But I'm still going to start it off as a private thing.

comment:7 Changed 6 years ago by exarkun

  • Status changed from new to assigned

comment:8 Changed 6 years ago by exarkun

This is looking like this at the moment:

from twisted.python._release import deprecation, twisted_8_1

@deprecation.decorator(twisted_8_1)
def foo(x):
    pass

I hope testing can still be done with callDeprecated but it's not clear how that will work yet.

comment:9 Changed 6 years ago by exarkun

Difficulties with testing:

  • callDeprecated takes a version instead of a release
  • the warning text emitted will probably be change after the first release the deprecation is included in
  • there's already assertWarns and callDeprecated and I feel bad adding a third API
  • All the stuff that the API needs to know about is in twisted.python._release. Putting a method into trial for it seems bad.

I'm leaning towards... something based on a general warning catching API, I guess? Still not really sure.

comment:10 Changed 6 years ago by jml

Random thoughts:

  • Re-use callDeprecated, have it send a deprecation signal if it is passed a version.
  • I don't think warning text needs to be explicitly part of the test. It's enough to test that it raises the right sort of warning.
  • I'd feel bad about a third API too.
  • This might be an argument for having a base test case that's there for Twisted-specific needs and doesn't make API promises to the rest of the world.
  • Or maybe it's an argument for liberating assertions from the shackles of TestCase objects.
  • In any case, it's not too bad to have domain-specific assertions in Trial. After all, it will make it easier to write good tests for things that change APIs.
  • I'd probably be happy with a general warning catching API that had specifics for our deprecation system.
  • IMO, the goal is to make it easy, even fun, to deprecate bad things.

comment:11 Changed 6 years ago by exarkun

  • Branch changed from branches/deprecation-tool-3266 to branches/deprecation-tool-3266-2

(In [24855]) Branching to 'deprecation-tool-3266-2'

comment:12 Changed 6 years ago by exarkun

Now I'm thinking about something like this:

from twisted.python.deprecate import _DeprecationPolicy, _Release
from twisted.python._release import deprecation, twisted_8_0

@deprecation.decorator(twisted_8_0)
def foo():
    pass
 
    ...

    def test_foo(self):
 
         foo()
 
         self.assertEqual(
             len(self.flushWarnings(
                     category=deprecation.deprecationType(twisted_8_0),
                     offendingFunction=self.test_foo)),
             1)

This implies some stuff:

  • deprecate most or all of twisted.python.deprecate
  • deprecate callDeprecated
  • deprecate assertWarns
  • add a better warning observer to trial
  • add support for emitting any non-flushed warnings at the end of each test

comment:13 Changed 6 years ago by exarkun

Maybe I'll document some motivation before I forget it:

  • callDeprecated and failUnlessWarns make dealing with the stack level difficult. Stack levels are fragile, and you don't really care about them anyway, you care about where the warning message ultimately points in your code. Having something like offendingFunction (perhaps to be augmented later with the addition of mutually exclusive offendingModule and offendingClass (any others?) lets tests express what they really care about. Of course, it's still a little redundant, since most often the test method itself will be the offending function, but this won't always be the case. (just as stacklevel=2 isn't always right)
  • The python warnings module is so hard to work with, having higher order functions is hard, because it suggests that certain things are possible (nesting, for example) when they really aren't.
  • This approach is more flexible, since the result is a data structure representing warnings, and each test can write the most appropriate assertions for the particular case being tested. failUnlessWarns and callDeprecated both suffer from the limitation that more than one warning causes them to flip out, and callDeprecated can only handle DeprecationWarnings (not PendingDeprecationWarning or any other warning category - although it likely doesn't care about non-deprecation warnings) and can't handle a different string message associated with the warning.
  • callDeprecated and failUnlessWarns also don't accept a release-like object, only a version, and they accept a version which represents the wrong thing (since the developer can't know what the right version to pass will be until a release following their development happens). This could be rectified by handling different types in each of these methods, but I started off trying that and it results in very ugly code which would involve emitting a deprecation for the old style /anyway/ eventually.
  • the deprecation object can eventually support many different kinds of deprecations (calling this function is deprecated, importing this module is deprecated, passing this argument to this function is deprecated, not defining this attribute on this object you passed to this function is deprecated, etc) and flushWarnings allows all of these to be tested. twisted.python.deprecate.deprecated would have difficulty with this.

comment:14 Changed 6 years ago by exarkun

  • Branch changed from branches/deprecation-tool-3266-2 to branches/deprecation-tool-3266-3

(In [24936]) Branching to 'deprecation-tool-3266-3'

comment:15 Changed 6 years ago by exarkun

(In [25013]) merge flushWarnings changes from #3266 branch

refs #3487
refs #3266

comment:16 Changed 5 years ago by exarkun

  • Status changed from assigned to new

comment:17 Changed 3 years ago by <automation>

  • Owner exarkun deleted

comment:18 Changed 18 months ago by tom.prince

#5645 is about documenting how to do deprecations, #6266 about suppressing the warning in tests of the deprecated function that aren't about deprecation.

comment:19 Changed 13 months ago by tom.prince

  • Keywords deprecation policy added; deprecation-policy removed
Note: See TracTickets for help on using tickets.