Ticket #6621 task new

Opened 10 months ago

Last modified 8 months ago

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

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

my-twisted-patch.patch Download (2.2 KB) - added by mulhern 10 months ago.
my-twisted-patch2.patch Download (2.5 KB) - added by mulhern 9 months ago.
my-twisted-patch3.patch Download (3.0 KB) - added by mulhern 9 months ago.

Change History

1

Changed 10 months ago by mulhern

  • component changed from core to mail

Changed 10 months ago by mulhern

2

Changed 10 months ago by mulhern

  • owner mulhern deleted
  • keywords review added

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 9 months ago by mulhern

3

Changed 9 months ago by mulhern

Second patch contains a .removal file.

4

Changed 9 months ago by thijs

  • description modified (diff)

Updating ticket description markup.

5

Changed 9 months ago by thijs

  • cc thijs added
  • owner set to mulhern
  • keywords review removed

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 9 months ago by mulhern

6

Changed 9 months ago by mulhern

  • owner mulhern deleted
  • keywords review added

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.

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

7

Changed 8 months ago by radix

  • owner set to mulhern
  • keywords review removed

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.