Opened 5 years ago

Last modified 4 years ago

#6547 defect new

Class mail.pb.MaildirBroker apparently broken and obsolete

Reported by: mulhern Owned by: radix
Priority: normal Milestone:
Component: mail Keywords:
Cc: lvh Branch: branches/deprecate-mail.pb-6547
branch-diff, diff-cov, branch-cov, buildbot
Author: radix, mulhern

Description

This class 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.

Note: MaildirBroker extends spread.pb.Broker which extends spread.banana.Banana which extends (internet.protocol.BaseProtocol which extends internet.protocol.Protocol) and twisted.styles.Ephemeral but not of these seem to define the missing fields.

Attached a unit test file which demonstrates the error by succeeding. It should be placed in the twisted/mail/test directory.

Attachments (4)

test_pb.py (1.1 KB) - added by mulhern 5 years ago.
my-twisted-patch.patch (1.3 KB) - added by mulhern 5 years ago.
my-twisted-patch1.patch (2.8 KB) - added by mulhern 5 years ago.
New patch deprecates instead of removes.
my-twisted-patch2.patch (2.2 KB) - added by mulhern 4 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by mulhern

Attachment: test_pb.py added

comment:1 Changed 5 years ago by mulhern

Owner: set to mulhern

Changed 5 years ago by mulhern

Attachment: my-twisted-patch.patch added

comment:2 Changed 5 years ago by mulhern

Keywords: review added
Owner: mulhern deleted

Removed the definition of the class MaildirBroker from twisted/mail/pb.py.

I'm removing it without deprecating it because I believe it "Fixes a Gross Violation of Specifications". The gross violation in this case is that if either method is called an AttributeError will be thrown and it is implicit that that should not happen.

comment:3 Changed 5 years ago by Thijs Triemstra

Keywords: review removed
Owner: set to mulhern

I don't see why we should make an exception here and removing it without deprecating it first. Can you do this? Feel free to join #twisted-dev on freenode to discuss things like this.

Changed 5 years ago by mulhern

Attachment: my-twisted-patch1.patch added

New patch deprecates instead of removes.

comment:4 Changed 5 years ago by mulhern

Keywords: review added
Owner: changed from mulhern to Thijs Triemstra

Class now deprecated instead of removed.

comment:5 Changed 4 years ago by Thijs Triemstra

Owner: Thijs Triemstra deleted

comment:6 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to mulhern

Thanks for working on this.

Looking at this further, there doesn't seem any reason to keep any of the code in this module. It looks like the start of a prototype that was never finished and never used. Thus, you should just deprecate the module.

  • t.p.hashlib is a good example of how to deprecate a module.
  • Note, there is no reason to assert that the methods raise. If somebody shows up and fixes the underlying code, there is no reason that this test should start failing.
  • The test module is missing a module level docstring
  • The replacement should be something like "a real mail protocol" or "IMAP".
  • The version will need to be bumped.

Archaeological note:

  • It looks like self.domains probably wants to be self.factory.domains instead.
  • The style of PB used appears to predate deferreds.

comment:7 Changed 4 years ago by mulhern

Keywords: review added
Owner: mulhern deleted

Thanks for your help.

I've uploaded a new patch that deprecates the whole module, a test that just tests for deprecation of the whole module, and an updated .removal file.

Changed 4 years ago by mulhern

Attachment: my-twisted-patch2.patch added

comment:8 Changed 4 years ago by radix

Author: mulhernradix, mulhern
Branch: branches/deprecate-mail.pb-6547

(In [39799]) Branching to deprecate-mail.pb-6547.

comment:9 Changed 4 years ago by radix

Sadly this patch was not merged before 13.1 was released, so it needs to be updated to specify the correct deprecation version.

I've done that in a new branch.

Builds here: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/deprecate-mail.pb-6547

comment:10 Changed 4 years ago by radix

Also fixed some flakes/pep8 stuff.

comment:11 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to radix
  • Looks like the news file is missing an is or a was
  • "please use a real mail protocol" sounds a bit unprofessional. Maybe "please use a standard email protocol such as POP or IMAP instead"
  • Consider using getDeprecationWarningString and callDeprecated to ensure that this is a standard assertion message. wiki:CompatibilityPolicy#Unittesting

Otherwise this looks fine.

Please merge.

Note: See TracTickets for help on using tickets.