Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#8082 enhancement closed fixed (fixed)

Document deprecation policy and how to implement a deprecation

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: release management Keywords:
Cc: radix Branch: branches/deprecation-docs-8082-2
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

We have a wiki page which is WIP https://twistedmatrix.com/trac/wiki/CompatibilityPolicy

We should move it into the narrative docs and make it official.

Change History (16)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: radix added

comment:2 Changed 4 years ago by Adi Roiban

Author: adiroiban
Branch: branches/deprecation-docs-8082

(In [46049]) Branching to deprecation-docs-8082.

comment:3 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Scope

This moved the WIP https://twistedmatrix.com/trac/wiki/CompatibilityPolicy wiki page to a final documentation which is part of Twisted documentation.

Changes

Most of the contest is a direct copy of the wiki page.

I have re-arranged the compatible and non-compatible examples.

I added example for how to use @deprecated and deprecatedModuleAttribute together with direct calls to warnings.warn

I have extended the unit testing part with examples of how to use those methods

I have inserted a reference to trial bug related to not failing on un-handled deprecation errors and I tried to add a workaround.

The wiki talks about a 3 phase deprecation process but it only describes 2 phases. I have updated the text to say 2 phases.

I would say the current way of checking deprecation warnings using self.flushWarnings() is bad and we should consolidate all those calls into a single helper method... but I have just went for recommendation found in the wiki page. This can be changed later.

How to test

Check that changes make sense... I have sent the branch to buildbot and there should be an HTML

comment:4 Changed 4 years ago by Adi Roiban

I forgot that I have also created #7894 for the same issue

comment:5 Changed 4 years ago by hawkowl

Keywords: review removed
Owner: set to Adi Roiban

Hi Adi, thanks for this.

Few notes:

  • Line 451 on the new docs, the flushWarnings should really specify the test case.
  • Line 419 down -- 1 sentence per line, please.
  • Line 509 -- Typo 'trail' -> 'Trial'.
  • The docstring probably wants to say 'now', not 'not'?
  • The index should have it title cased ("Compatibility Policy")

Please fix these issues and put up for re-review. Thanks! <3

comment:6 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

Thanks for the review.

Please check latest changes.

Line 451 on the new docs, the flushWarnings should really specify the test case.

True. I have also updated the docstring.

Line 419 down -- 1 sentence per line, please.

Done

Line 509 -- Typo 'trail' -> 'Trial'.

Done.

The docstring probably wants to say 'now', not 'not'?

At which line? At 510 is 'not'. As tests will not fail.

The index should have it title cased ("Compatibility Policy")

Done.

comment:7 Changed 4 years ago by hawkowl

Keywords: review removed
Owner: set to Adi Roiban

Hi Adi, thanks for this.

Rather than going through a few more cycles for minor changes, I went in and did them on the branch -- please check over my commits, and merge if you're happy, otherwise change them and put up for review.

Thanks! <3

comment:8 Changed 4 years ago by Adi Roiban

Many thanks for the changes. All good... but I found that the @deprecated decorator does not play nice with @property decorator.

I don't know how to fix it.

Please suggest a possible fix.

Thanks!

comment:9 Changed 4 years ago by hawkowl

#8124 is the ticket following up on the @property fixes.

comment:10 Changed 3 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have updated the docs now that #8124 is fixed.

How to deprecate instance attributes for old style classes ?

Please check that class deprecation is the right way to do it... not sure if deprecateModuleAttirbute is the right way and whether it should be call before the class definition, inside the class or after the class :)

Please review.

comment:11 Changed 3 years ago by Glyph

Just spun the builders to check syntax &c

comment:12 in reply to:  10 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to Adi Roiban

Thanks for contributing this, Adi. You're absolutely right that this should move into official policy documentation and stop rotting on the wiki. When this lands, please delete the wiki page and replace it with a link to the document :).

Replying to adiroiban:

I have updated the docs now that #8124 is fixed.

Please merge forward so that the builders can succeed :-\.

How to deprecate instance attributes for old style classes ?

Right now, there's no way. I think it's fine not to talk about it; this document can land before being perfect. File a new ticket for this functionality and let's discuss there.

Please check that class deprecation is the right way to do it... not sure if deprecateModuleAttirbute is the right way and whether it should be call before the class definition, inside the class or after the class :)

This is correct documentation of the state of the art. It should not be called inside the class, but either before or after should be OK.

However, it would be better if @deprecated did the work of setting up deprecatedModuleAttribute for you, since both importing and calling a deprecated name should raise warnings. Separate ticket though! :) Now that these docs are landed, we can have that ticket update the narrative documentation explaining how to use it as well, which is all the more reason to land this change!

Please review.

This is looking very good. Please merge forward and if you don't encounter any issues getting an all-green build, go ahead and land.

comment:13 Changed 3 years ago by Adi Roiban

Branch: branches/deprecation-docs-8082branches/deprecation-docs-8082-2

(In [47264]) Branching to deprecation-docs-8082-2.

comment:14 Changed 3 years ago by Adi Roiban

Thanks for the review.

I have created #8297 to follow up on deprecated instances for old style classes.

Looking forward for the buildbot results and will merge.

comment:15 Changed 3 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [47279]) Merge deprecation-docs-8082-2: Document the deprecation policy.

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

comment:16 Changed 3 years ago by Adi Roiban

I think that wiki page should be updated after the release... for now I left a comment at the top of that page

Note: See TracTickets for help on using tickets.