Remove deprecated IDomain.startMessage

[thijs]

IDomain.startMessage in trunk/twisted/mail/mail.py is deprecated, let's remove it.

[thijs]

(In [28016]) Branching to 'remove-idomain-startmessage-4151'

[thijs]

(In [28017]) Remove IDomain.startMessage, refs #4151

[thijs]

(In [28018]) Coding standard, refs #4151

[moijes12]

here's the patch.

[moijes12]

here's the patch.

[thijs]

Thanks for your patch.

1. Could you also bring the method's class it's docstrings up to par with the coding standard? Docstrings should be on 3 lines, newlines between classes and methods etc. See r28018 in the branch where I started doing that.
2. The .removal file should mention the version or year that startMessage was deprecated.
3. Several of the docstrings mention twisted.protocols.smtp which doesn't exist anymore

[moijes12]

Hi. I've got some questions here :

1. For points 1 and 3 of the previous review
   
   1. I can and I could but would we need to raise another ticket for that ?
      

[rodrigc]

I am a newcomer to Twisted, and am participating in the Twisted Sprint in San Francisco, on 8/19/2012.

There are two issues to this ticket that I see: (1) The changes to the docstrings are legitimate changes, but should

be done under a separate ticket, and not intermingled with changes to remove the deprecated code.

(2) It is not OK to delete the deprecated method in this case.

You need to follow the procedure at http://twistedmatrix.com/trac/wiki/CompatibilityPolicy

For example, you need to invoke warnings.warn() in the deprecated method, before you can remove the method.

Also, with this patch, there is no accompany patch to any of the unit tests. There should probably be a patch to unit tests.

[thijs]

When you do a review, make sure to remove the keyword and assign it back.

[moijes12]

• In my last comment I did mention that it should be under a separate ticket, but I think it can be done here.
• Well, it is ok to delete the delete the deprecated method and thats what the ticket is for.

[tom.prince]

It looks like this function is still used:

From twisted/mail/protocols.py:

ret.append(self.service.domains[user.dest.domain].startMessage(user))

so I'm not sure that it is appropriate to remove this method.

[exarkun]

(In [36620]) Branching to 'remove-idomain-startmessage-4151-2'

[exarkun]

Thanks, everyone, to your work on this so far.

Sometimes it's necessary for library code to keep calling a deprecated method up until the point when the method is finally deleted (otherwise, applications wouldn't really be warned about the deprecation prior to their code no longer working). This deprecation wasn't very pro-active (you had to read an interface docstring instead of looking at warnings output from running your test suite), but that's the way a lot of really old deprecations were done. We generally grandfather these in (but all new warnings should be emitted using the warnings module), partly because they have been there for so long - developers have had plenty of warning to stop relying on this functionality, even without a warning being emitted.

This branch needs to actually clean up the rest of the uses of startMessage in the code-base, though. Deleting the method definition from the interface is a good start, but now the associated implementation is no longer necessary and should be removed as well. Also, the unit tests for that implementation. If there is any application code in Twisted that is still relying on the functionality, that needs to be fixed as well. Ideally, this would have happened already. Since it seems it has not.... There are actually quite a few places in Twisted Mail that may need to be updated for this change:

• twisted.mail.relay.DomainQueuer.exists calls DomainQueuer.startMessage. This is actually fine, I think. This startMessage implementation will become an implementation detail of DomainQueuer, rather than an implementation of the interface. exists is still part of the interface, the functionality will continue to work.
• twisted.mail.alias.AddressAlias.createMessageReceiver calls startMessage on an externally supplied domain object. The appropriate replacement is probably IDomain.exists, which is somewhat like validateTo (but validateTo is not part of IDomain so it cannot be used here).
• twisted.mail.maildir.AbstractMaildirDomain.exists looks like it falls into the same boat as twisted.mail.relay.DomainQueuer.exists.
• Three other uses can probably just be deleted:
  • twisted.mail.protocols.DomainDeliveryBase.startMessage
  • twisted.mail.mail.BounceDomain.startMessage
  • twisted.mail.smtp.SMTP.startMessage

This all seems like fair game for this ticket, as none of the changes are terribly huge. I suspect some testing improvements will be necessary as well.

[thijs]

(In [36826]) address review comments, refs #4151

[thijs]

I think all of it was covered, please review.

[tom.prince]

1. twisted.mail.maildir.AbstractMaildirDomain.startMessage is still public, even if not part of the interface, so probably shouldn't be removed. In any case,
   1. it makes sense to keep factored, even if it gets made private
   2. a new file shouldn't be created until the function returned from exists is called (and there is nothing indicating that this method will only be called once).
2. The same reasoning can be applied to twisted.mail.relay.DomainQueuer.exists calls DomainQueuer.startMessage.

I haven't looked at the tests yet.

[tom.prince]

Thanks.

1. There needs to be at least a test that AddressAlias.createMessageReceiver has the right behavior.
2. If 2.2 from comment 17 is addressed, then the changes to RelayTestCase are superfluous.

Please fix the above (and points from comment 17) and resubmit for review.

A couple of additional pints, that would be nice to have:

3. Given 2, it appears that neither remaining startMessage implementation is unit tested. (LiveFireExercise does do some integration tests on it).
4. It would be good to test thta AbstractMaildirDomain.exists and DomainQueuer.exists both return a callable that can be called multiple times to create distinct messages (both in terms of objects, and the underlying storage backing those messages).

[thijs]

(In [37189]) address review comments, refs #4151

[thijs]

Replying to tom.prince:

1. twisted.mail.maildir.AbstractMaildirDomain.startMessage is still public, even if not part of the interface, so probably shouldn't be removed. In any case, 1. it makes sense to keep factored, even if it gets made private

Moved it to a private method with the same name. Should startMessage be deprecated here?

1. a new file shouldn't be created until the function returned from exists is called (and there is nothing indicating that this method will only be called once).

Not sure what you mean here..

1. The same reasoning can be applied to twisted.mail.relay.DomainQueuer.exists calls DomainQueuer.startMessage.

Moved the functionality to a private method, and same question here, should startMessage be deprecated here first (although this ticket will get rid of it's interface method..)

1. There needs to be at least a test that AddressAlias.createMessageReceiver has the right behavior.

Added uncovered tests for AddressAlias.

[tom.prince]

1. twisted.mail.maildir.AbstractMaildirDomain.startMessage is still public, even if not part of the interface, so probably shouldn't be removed. In any case, 1. it makes sense to keep factored, even if it gets made private

Moved it to a private method with the same name. Should startMessage be deprecated here?

I don't see any strong reason to make it private, or deprecate it.

1. a new file shouldn't be created until the function returned from exists is called (and there is nothing indicating that this method will only be called once).

Not sure what you mean here.

createMessage = maildir.exists(...)
# No file should have been created yet
msg1 = createMessage()
# One file
msg2 = createMessage()
# Two files

The way you had changed the code (the code in the branch works now) created a single file, and the created multiple MaildirMessages pointing at the same file object.

Please commit after addressing the following points and running things through buildbot.

1. Remove the private aliasing of the two startMessage implementations. (If you think they should be deprecated, please file a separate ticket for doing that)
2. File a ticket to address points 3+4 of comment:18.
3. Revert the changes to RelayTestCase. (They'll be needed for the ticket in 2, though).

[thijs]

(In [37668]) Branching to 'remove-idomain-startmessage-4151-3'

[thijs]

(In [37669]) merge forward, refs #4151

[thijs]

Replying to tom.prince:

Please commit after addressing the following points and running things through buildbot.

Forced a new  build.

1. Remove the private aliasing of the two startMessage implementations. (If you think they should be deprecated, please file a separate ticket for doing that)

Removed.

2. File a ticket to address points 3+4 of comment:18.

Opened #6383.

3. Revert the changes to RelayTestCase. (They'll be needed for the ticket in 2, though).

Done.

Will merge when builder's green.

[thijs]

(In [37670]) Merge remove-idomain-startmessage-4151-3: mail.IDomain.startMessage, deprecated since 2003, is removed now.

Author: thijs, moijes12 Reviewer: rodrigc, tom.prince, exarkun Fixes: #4151