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

[brandl]

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

Please deprecate them.

[brandl]

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

[brandl]

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?

[thijs]

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.

[rwall]

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?

[exarkun]

I think deprecatedSince should be 10.1.1

10.2 :)

[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.

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.

[brandl]

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

[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.

[exarkun]

• 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.

[exarkun]

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

[exarkun]

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

refs #4541

[exarkun]

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

refs #4541

[exarkun]

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

refs #4541

[exarkun]

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

refs #4541

[exarkun]

(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).