Opened 4 years ago

Closed 4 years ago

#4541 defect closed fixed (fixed)

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

ticket_4541.1.patch (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 (3.8 KB) - added by brandl 4 years ago.
Added news entry (forgot to call "svn add" :-( )

Download all attachments as: .zip

Change History (16)

Changed 4 years ago by brandl

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

comment:1 follow-up: Changed 4 years ago by brandl

  • Keywords review added
  • Owner changed from brandl to exarkun

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?

comment: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.

comment:3 Changed 4 years ago by rwall

  • Keywords review removed
  • Owner set to brandl

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?

comment:4 Changed 4 years ago by exarkun

I think deprecatedSince should be 10.1.1

10.2 :)

comment:5 follow-up: 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" :-( )

comment: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.

comment:7 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner changed from rwall to 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.

comment:8 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/deprecate-old-netstring-attributes-4541

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

comment:9 Changed 4 years ago by exarkun

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

refs #4541

comment: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

comment: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

comment:12 Changed 4 years ago by exarkun

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

refs #4541

comment:13 Changed 4 years ago by exarkun

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

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

comment:14 Changed 4 years ago by <automation>

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