Opened 9 years ago

Closed 8 years ago

#6486 task closed fixed (fixed)

Replace usage of has_key() in twisted.mail

Reported by: Thijs Triemstra Owned by: Richard Wall
Priority: low Milestone: Python-3.x
Component: mail Keywords: easy
Cc: Thijs Triemstra Branch: branches/has_key-6486-2
branch-diff, diff-cov, branch-cov, buildbot
Author: mulhern, lvh, rwall

Description

The following modules in twisted.mail contain calls to the deprecated x.has_key(y) method. They should be replaced with y in x instead. They're all covered by tests already, except for twisted.mail.pb which also doesn't show up in the coverage report?

./twisted/mail/mail.py:36:    def has_key(self, name):
./twisted/mail/pb.py:102:        if not self.domains.has_key(domain):
./twisted/mail/pb.py:105:        if (domain.dbm.has_key(name) and
./twisted/mail/test/test_mail.py:84:            self.failUnless(d.has_key(x))
./twisted/mail/test/test_mail.py:723:         self.failUnless(m.has_key('Received'))
./twisted/mail/maildir.py:480:        if not self.dbm.has_key(name):

Attachments (3)

my-twisted-patch.patch (2.7 KB) - added by mulhern 9 years ago.
my-twisted-patch.2.patch (1.7 KB) - added by mulhern 9 years ago.
New patch
has_key_6486.patch (2.7 KB) - added by Lasse 8 years ago.
Added documentation

Download all attachments as: .zip

Change History (21)

comment:1 Changed 9 years ago by mulhern

The occurrence of has_key in mail/mail.py : 36 is a definition of a class method for the class DomainWithDefaultDict.

DomainWithDefaultDict also contains an contains method (implementing in).

Should the definition of the has_key method in DomainWithDefaultDict be removed? or does it constitute part of a public interface?

comment:2 Changed 9 years ago by Itamar Turner-Trauring

Anything that is a public (doesn't start with underscore) method of a public class cannot be removed or otherwise made non-backwards compatible unless it's been deprecated for more than a year and two releases.

comment:3 Changed 9 years ago by mulhern

Owner: set to mulhern
Status: newassigned

Changed 9 years ago by mulhern

Attachment: my-twisted-patch.patch added

comment:4 Changed 9 years ago by mulhern

Keywords: review added
Owner: mulhern deleted
Status: assignednew

Removed the class MaildirBroker In twisted/mail/pb.py because it appears to be broken and obsolete. Here are the reasons: 1) No unit tests. 2) Last change to the file was two years ago. 3) Most recent update to code in this class is revision 581. 4) Running either of the two methods getCollections or proto_getCollections on a MaildirBroker object results in an AttributeError. The attribute errors indicate that the field "domains" or the field "_getCollection" do not exist in the object. 5) No occurrences of the name MaildirBroker exist anywhere else in the codebase.

Did not change test_mail.py:84 since it is there explicitly to test the has_key function.

comment:5 Changed 9 years ago by lvh

Owner: set to lvh

comment:6 Changed 9 years ago by lvh

Keywords: review removed
Owner: changed from lvh to mulhern

Hi mulhern,

Thanks again for your contribution :)

Unfortunately, the compatibility policy prevents us from yanking it out, even if it's broken and despite all the issues with it you highlighted.

If MaildirBroker is busted, file a ticket for it. According to the compatibility policy, if it's an egregious failure of specification, it can be fixed immediately. If it's obsolete (why?) and needs to be removed, since it's public API, that's the 1 year and 2 release minimum there (as itamar mentioned).

Otherwise, this patch looks okay -- reinstate MaildirBroker and I think we're done :)

thanks again, lvh

Changed 9 years ago by mulhern

Attachment: my-twisted-patch.2.patch added

New patch

comment:7 Changed 9 years ago by mulhern

Keywords: review added; easy removed
Owner: changed from mulhern to lvh

New patch omits change to twisted.mail.pb.py and new ticket (#6547) submitted regarding MaildirBroker's presumed obsolescence.

comment:8 Changed 9 years ago by lvh

Author: lvh
Branch: branches/has_key-6486

(In [38643]) Branching to 'has_key-6486'

comment:10 Changed 9 years ago by lvh

Keywords: easy added; review removed

There's a remaining issue with your patch:

        self.assertIsNone(self.D.userDirectory('nouser')) 

assertIsNone isn't a thing: you probably mean:

        self.assertIdentical(self.D.userDirectory('nouser'), None) 

Since I missed it in the first review, and it's obvious what you meant, I'll commit the appropriate thing. Unfortunately that means I can't be the next reviewer :)

comment:11 Changed 9 years ago by lvh

Keywords: review added
Owner: lvh deleted

Last patch was committed to the branch and then I added two commits: r38648, r38649. State of the branch is what's up for review.

Buildbot: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/has_key-6486

comment:12 in reply to:  11 Changed 9 years ago by Thijs Triemstra

Author: lvhmulhern, lvh
Keywords: review removed
Owner: set to mulhern

Replying to lvh:

Last patch was committed to the branch and then I added two commits: r38648, r38649. State of the branch is what's up for review.

Thanks. There are a few new twistedchecker errors, assigning back to mulhern.

Changed 8 years ago by Lasse

Attachment: has_key_6486.patch added

Added documentation

comment:13 Changed 8 years ago by Lasse

Keywords: review added

comment:14 Changed 8 years ago by Alex Gaynor

On twisted/mail/maildir.py:L480, I think you want to replace not name in self.dbm with name not in self.dbm

comment:15 Changed 8 years ago by Richard Wall

(In [40068]) Apply has_key_6486.patch from lbromo. refs #6486

comment:16 Changed 8 years ago by Richard Wall

Author: mulhern, lvhmulhern, lvh, rwall
Branch: branches/has_key-6486branches/has_key-6486-2

(In [40079]) Branching to 'has_key-6486-2'

comment:17 in reply to:  14 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: changed from mulhern to Richard Wall
Status: newassigned

Hi lbromo,

  • I applied your patch.
  • Addressed AlexGaynor's comment.
  • Modified one or two of the new docstrings
  • Added a newsfile.
  • Merged forward.

The build results look good.

So I'll merge it. Thanks.

comment:18 Changed 8 years ago by Richard Wall

Resolution: fixed
Status: assignedclosed

(In [40095]) Merge has_key-6486-2

Authors: mulhern, lbromo Reviewers: lvh, rwall, thijs, Alex Fixes: #6486

Remove use of has_key in twisted.mail.maildir and twisted.mail.test.test_mail. Added some missing test docstrings in twisted.mail.test.test_mail.

Note: See TracTickets for help on using tickets.