Ticket #4541 defect closed fixed

Opened 4 years ago

Last modified 4 years ago

Deprecate unused module attributes in twisted.protocols.basic.py

Reported by: brandl Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs Branch: branches/deprecate-old-netstring-attributes-4541
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

Due to changes in t.p.b.NetstringReceiver, the module attributes LENGTH, DATA, COMMA, and NUMBER are no longer used.

Please deprecate them.

Attachments

ticket_4541.1.patch Download (1.3 KB) - added by brandl 4 years ago.
A patch against the trunk deprecating the attributes LENGTH, DATA, COMMA, and NUMBER
ticket_4541.2.patch Download (3.8 KB) - added by brandl 4 years ago.
Added news entry (forgot to call "svn add" :-( )

Change History

Changed 4 years ago by brandl

A patch against the trunk deprecating the attributes LENGTH, DATA, COMMA, and NUMBER

1

follow-up: ↓ 2   Changed 4 years ago by brandl

  • owner changed from brandl to exarkun
  • keywords review added

I've attached a patch for deprecating these attributes. Not sure if the deprecation version is correct, though.

Please check.

BTW: I'm not too familiar with the workflow of tickets - is it OK to assign these things directly to you, or should I just change the keyword to "review" and wait until someone grabs the issue?

2

in reply to: ↑ 1   Changed 4 years ago by thijs

  • cc thijs added
  • owner exarkun deleted

Replying to brandl:

BTW: I'm not too familiar with the workflow of tickets - is it OK to assign these things directly to you, or should I just change the keyword to "review" and wait until someone grabs the issue?

Thanks for the patch, see TwistedDevelopment for more info.

3

  Changed 4 years ago by rwall

  • owner set to brandl
  • keywords review removed

Code Review

Looks mostly fine to me - tests pass, warnings are printed upon accessing those module vars.

A few problems:

  1. According to the policy, you need to add unit tests for deprecations (see  http://twistedmatrix.com/trac/wiki/CompatibilityPolicy#Unittesting)
  2. Missing news file (see  http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles)
  3. I think deprecatedSince should be 10.1.1 (See  http://twistedmatrix.com/trac/wiki/CompatibilityPolicy#Functionsandmethods "the version of Twisted in which it was first deprecated"
  4. I'd say you should either delete those left over module level private vars (and basic.attrib, basic.reason) afterwards, OR
  5. ....just keep it simple (and clearer) by writing three separate deprecate.deprecatedModuleAttribute calls instead of the loop.
  6. "NUMBER", _useInstead % ("_LENGTH",): I don't know NetstringReceiver very well, but is that the correct alternative?

4

  Changed 4 years ago by exarkun

I think deprecatedSince should be 10.1.1

10.2 :)

5

follow-up: ↓ 6   Changed 4 years ago by brandl

  • keywords review added
  • owner changed from brandl to rwall

Thanks for the review. I've uploaded a second patch.

There is a new unit test checking for the deprecated module attributes and I have added a news file. The code was simplified according to your suggestions, and the _deprecatedSince attribute is deleted after its use now.

I've changed the version number for deprecatedSince to 10.2.0, following the comment by exarkun. I'm still not sure how to determine this number, though. Is this the version of Twisted that includes the fix for  http://twistedmatrix.com/trac/ticket/4378? Starting with this version, the module attributes should not be used anymore.

Is there a simple way to determine in which version ticket 4378 will be fixed? I didn't find it in the tickets for Milestone 10.2, and there is no Milestone for 10.1.1.

Oh, and yes: NUMBER was replaced by NetstringReceiver._LENGTH. They both represent a match object for parsing the length specification of a netstring. Maybe _LENGTH_PATTERN would have been a better name?

Please check.

Changed 4 years ago by brandl

Added news entry (forgot to call "svn add" :-( )

6

in reply to: ↑ 5   Changed 4 years ago by rwall

Replying to brandl:

Thanks for the review. I've uploaded a second patch. There is a new unit test checking for the deprecated module attributes and I have added a news file. The code was simplified according to your suggestions, and the _deprecatedSince attribute is deleted after its use now.

Great, it looks much clearer to me.

I've changed the version number for deprecatedSince to 10.2.0, following the comment by exarkun. I'm still not sure how to determine this number, though. Is this the version of Twisted that includes the fix for  http://twistedmatrix.com/trac/ticket/4378? Starting with this version, the module attributes should not be used anymore. Is there a simple way to determine in which version ticket 4378 will be fixed? I didn't find it in the tickets for Milestone 10.2, and there is no Milestone for 10.1.1.

Not sure. I'll leave someone else to answer that.

Oh, and yes: NUMBER was replaced by NetstringReceiver._LENGTH. They both represent a match object for parsing the length specification of a netstring. Maybe _LENGTH_PATTERN would have been a better name?

Understood.

Please check.

Looks good. Tests pass. Only one thing:

richard@aziz:~/Projects/Twisted/trunk$ pyflakes twisted/test/test_protocols.py
twisted/test/test_protocols.py:614: local variable 'length' is assigned to but never used
twisted/test/test_protocols.py:625: local variable 'length' is assigned to but never used

...which may be annoying for anyone looking at the automated pyflakes buildbot output. I guess you don't need to assign the module variables in order to trigger the warning.

You'll have to get one of the twisted devs (eg exarkun) to merge this code to trunk, so see what they say about this.

7

  Changed 4 years ago by exarkun

  • owner changed from rwall to exarkun
  • keywords review removed
  • For deprecations, please use a .removal news fragment.
  • The deprecation warnings should not point people to private attributes. There is no public replacement for these attributes, that's fine.
  • Indeed, the deprecation warning can be tested just by reading the attribute, no local needs to be assigned to.

8

  Changed 4 years ago by exarkun

  • branch set to branches/deprecate-old-netstring-attributes-4541
  • branch_author set to exarkun

(In [29957]) Branching to 'deprecate-old-netstring-attributes-4541'

9

  Changed 4 years ago by exarkun

(In [29958]) apply ticket_4541.2.patch

refs #4541

10

  Changed 4 years ago by exarkun

(In [29959]) Change the deprecation messages to not point people at implementation details and just warn them away instead.

refs #4541

11

  Changed 4 years ago by exarkun

(In [29960]) Fix two pyflakes warnings by adding some asserts about the return value of _consumeLength

refs #4541

12

  Changed 4 years ago by exarkun

(In [29961]) Oops; no, _consumeLength returns None

refs #4541

13

  Changed 4 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(In [29962]) Merge deprecate-old-netstring-attributes-4541

Author: brandl, exarkun Reviewer: rwall, exarkun Fixes: #4541

Deprecate the twisted.protocols.basic module attributes which were only in support of the old NetstringReceiver parser (no longer in use since #4378 was resolved).

14

  Changed 3 years ago by <automation>

  • owner exarkun deleted
Note: See TracTickets for help on using tickets.