Opened 5 years ago

Closed 19 months ago

#4151 task closed fixed (fixed)

Remove deprecated IDomain.startMessage

Reported by: thijs Owned by: thijs
Priority: normal Milestone:
Component: mail Keywords:
Cc: moijes12@… Branch: branches/remove-idomain-startmessage-4151-3
(diff, github, buildbot, log)
Author: thijs, exarkun Launchpad Bug:

Description

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

Attachments (3)

4151.patch (816 bytes) - added by moijes12 2 years ago.
here's the patch.
4151.2.patch (816 bytes) - added by moijes12 2 years ago.
here's the patch.
4151.3.patch (4.7 KB) - added by moijes12 2 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 5 years ago by thijs

  • Author set to thijs
  • Branch set to branches/remove-idomain-startmessage-4151

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

comment:2 Changed 5 years ago by thijs

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

comment:3 Changed 5 years ago by thijs

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

comment:4 Changed 4 years ago by <automation>

  • Owner exarkun deleted

Changed 2 years ago by moijes12

here's the patch.

Changed 2 years ago by moijes12

here's the patch.

comment:5 Changed 2 years ago by moijes12

  • Cc moijes12@… added
  • Keywords review easy added

comment:6 Changed 2 years ago by thijs

  • Keywords review removed
  • Owner set to moijes12

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

comment:7 Changed 2 years ago by 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 ?

Changed 2 years ago by moijes12

comment:8 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

comment:9 Changed 2 years ago by 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.

comment:10 Changed 2 years ago by thijs

  • Keywords review removed
  • Owner set to moijes12

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

comment:11 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted
  • 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.

comment:12 Changed 22 months ago by 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.

comment:13 Changed 22 months ago by exarkun

  • Author changed from thijs to thijs, exarkun
  • Branch changed from branches/remove-idomain-startmessage-4151 to branches/remove-idomain-startmessage-4151-2

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

comment:14 Changed 22 months ago by exarkun

  • Keywords review removed
  • Owner set to thijs

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.

comment:15 Changed 21 months ago by thijs

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

comment:16 Changed 21 months ago by thijs

  • Keywords review added
  • Owner thijs deleted

I think all of it was covered, please review.

comment:17 Changed 21 months ago by 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.

comment:18 follow-up: Changed 21 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs

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:

  1. Given 2, it appears that neither remaining startMessage implementation is unit tested. (LiveFireExercise does do some integration tests on it).
  2. 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).

comment:19 Changed 21 months ago by thijs

  • Status changed from new to assigned

comment:20 Changed 20 months ago by thijs

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

comment:21 in reply to: ↑ 18 Changed 20 months ago by thijs

  • Keywords review added; easy removed
  • Owner thijs deleted
  • Status changed from assigned to new

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.

comment:22 follow-up: Changed 20 months ago by tom.prince

  • Keywords review removed
  • Owner set to thijs
  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).

comment:23 Changed 19 months ago by thijs

  • Branch changed from branches/remove-idomain-startmessage-4151-2 to branches/remove-idomain-startmessage-4151-3

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

comment:24 Changed 19 months ago by thijs

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

comment:25 in reply to: ↑ 22 Changed 19 months ago by thijs

  • Status changed from new to assigned

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.

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

Opened #6383.

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

Done.

Will merge when builder's green.

comment:26 Changed 19 months ago by thijs

  • Resolution set to fixed
  • Status changed from assigned to closed

(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

Note: See TracTickets for help on using tickets.