Opened 13 years ago
Closed 9 years ago
#4151 task closed fixed (fixed)
Remove deprecated IDomain.startMessage
Reported by: | Thijs Triemstra | Owned by: | Thijs Triemstra |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Keywords: | ||
Cc: | moijes12 | Branch: |
branches/remove-idomain-startmessage-4151-3
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs, exarkun |
Description
IDomain.startMessage
in [source:trunk/twisted/mail/mail.py] is deprecated, let's remove it.
Attachments (3)
Change History (29)
comment:1 Changed 12 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/remove-idomain-startmessage-4151 |
comment:4 Changed 11 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
comment:5 Changed 10 years ago by
Cc: | moijes12 added |
---|---|
Keywords: | review easy added |
comment:6 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks for your patch.
- 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.
- The .removal file should mention the version or year that
startMessage
was deprecated. - Several of the docstrings mention
twisted.protocols.smtp
which doesn't exist anymore
comment:7 Changed 10 years ago by
Hi. I've got some questions here :
- For points 1 and 3 of the previous review
- I can and I could but would we need to raise another ticket for that ?
- I can and I could but would we need to raise another ticket for that ?
Changed 10 years ago by
Attachment: | 4151.3.patch added |
---|
comment:8 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
comment:9 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
Author: | thijs → thijs, exarkun |
---|---|
Branch: | branches/remove-idomain-startmessage-4151 → branches/remove-idomain-startmessage-4151-2 |
(In [36620]) Branching to 'remove-idomain-startmessage-4151-2'
comment:14 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
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
callsDomainQueuer.startMessage
. This is actually fine, I think. ThisstartMessage
implementation will become an implementation detail ofDomainQueuer
, 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
callsstartMessage
on an externally supplied domain object. The appropriate replacement is probablyIDomain.exists
, which is somewhat likevalidateTo
(butvalidateTo
is not part ofIDomain
so it cannot be used here).twisted.mail.maildir.AbstractMaildirDomain.exists
looks like it falls into the same boat astwisted.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:16 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Thijs Triemstra deleted |
I think all of it was covered, please review.
comment:17 Changed 9 years ago by
twisted.mail.maildir.AbstractMaildirDomain.startMessage
is still public, even if not part of the interface, so probably shouldn't be removed. In any case,- it makes sense to keep factored, even if it gets made private
- 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).
- 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: 21 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
Thanks.
- There needs to be at least a test that
AddressAlias.createMessageReceiver
has the right behavior. - 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:
- Given 2, it appears that neither remaining
startMessage
implementation is unit tested. (LiveFireExercise
does do some integration tests on it). - It would be good to test thta
AbstractMaildirDomain.exists
andDomainQueuer.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 9 years ago by
Status: | new → assigned |
---|
comment:21 Changed 9 years ago by
Keywords: | review added; easy removed |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | assigned → new |
Replying to tom.prince:
twisted.mail.maildir.AbstractMaildirDomain.startMessage
is still public, even if not part of the interface, so probably shouldn't be removed. In any case,
- 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?
- 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..
- 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..)
- There needs to be at least a test that
AddressAlias.createMessageReceiver
has the right behavior.
Added uncovered tests for AddressAlias
.
comment:22 follow-up: 25 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
- 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.
- 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 MaildirMessage
s pointing at the same file
object.
Please commit after addressing the following points and running things through buildbot.
- Remove the private aliasing of the two
startMessage
implementations. (If you think they should be deprecated, please file a separate ticket for doing that) - File a ticket to address points 3+4 of comment:18.
- Revert the changes to
RelayTestCase
. (They'll be needed for the ticket in 2, though).
comment:23 Changed 9 years ago by
Branch: | branches/remove-idomain-startmessage-4151-2 → branches/remove-idomain-startmessage-4151-3 |
---|
(In [37668]) Branching to 'remove-idomain-startmessage-4151-3'
comment:25 Changed 9 years ago by
Status: | new → assigned |
---|
Replying to tom.prince:
Please commit after addressing the following points and running things through buildbot.
Forced a new build.
- 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.
- File a ticket to address points 3+4 of comment:18.
Opened #6383.
- 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 9 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [28016]) Branching to 'remove-idomain-startmessage-4151'