Opened 16 months ago

Last modified 14 months ago

#6621 task new

method has_key in twisted.mail.mail.DomainWithDefaultDict redundant and deprecated

Reported by: mulhern Owned by: mulhern
Priority: low Milestone:
Component: mail Keywords:
Cc: thijs Branch:
Author: Launchpad Bug:

Description (last modified by thijs)

This method has the same purpose and semantics as the __contains__ method in the same class. The has_key method should be deprecated so that it can eventually be removed.

Attachments (3)

my-twisted-patch.patch (2.2 KB) - added by mulhern 16 months ago.
my-twisted-patch2.patch (2.5 KB) - added by mulhern 16 months ago.
my-twisted-patch3.patch (3.0 KB) - added by mulhern 15 months ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 16 months ago by mulhern

  • Component changed from core to mail

Changed 16 months ago by mulhern

comment:2 Changed 16 months ago by mulhern

  • Keywords review added
  • Owner mulhern deleted

I've added a deprecation warning to the method and a test for the deprecation. I've moved the existing test for correctness of the has_key method to the new method.

Changed 16 months ago by mulhern

comment:3 Changed 16 months ago by mulhern

Second patch contains a .removal file.

comment:4 Changed 15 months ago by thijs

  • Description modified (diff)

Updating ticket description markup.

comment:5 Changed 15 months ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to mulhern

Thanks for your patch.

Couple of minor things:

  1. In twisted/mail/test/test_mail.py:
    1. You remove the has_key assertion but we still want to test this functionality using self.failUnless(x in d)
    2. test_has_key should be test_hasKey
  2. In twisted/mail/mail.py:
    1. Twisted 13.1.0 has already been released, so the version nr should be bumped to 13.2.0
    2. The has_key method needs a docstring
  3. I'd make the new file a little easier to read:
    twisted.mail.mail.DomainWithDefaultDict.has_key method is deprecated now in favor of the __contains__ method. 
    

Changed 15 months ago by mulhern

comment:6 Changed 15 months ago by mulhern

  • Keywords review added
  • Owner mulhern deleted

Thanks for your comments.

1.1 self.failUnless test was moved into test_hasKey. Then when has_key goes away so can the whole test method and nothing else.

.2 test_has_key is now test_hasKey.

2.1 Version number bumped.

.2 has_key method now has 5 whole lines of description.

  1. removes file is more verbose and probably friendlier and more explanatory.

comment:7 Changed 14 months ago by radix

  • Keywords review removed
  • Owner set to mulhern

Sorry for the late review.

  1. I think you misunderstood the point of the 'return 1' implementation -- it's not because subclasses are expected, but rather because logically *all* domains exist given that any name has a default value.
  2. Your news file is messed up. Looks like you appended to it instead of replacing it.

I didn't notice any other problems with the change.

Note: See TracTickets for help on using tickets.