Opened 10 years ago
Closed 9 years ago
#5852 task closed fixed (fixed)
Deprecating tlsMode
Reported by: | argonemyth | Owned by: | Richard Wall |
---|---|---|---|
Priority: | low | Milestone: | |
Component: | Keywords: | smtp ESMTPClient tlsMode | |
Cc: | Nam Nguyen, moijes12, Richard Wall | Branch: |
branches/deprecate-tlsmode-5852
branch-diff, diff-cov, branch-cov, buildbot |
Author: | moijes12, rwall |
Description
The class attribute tlsMode
in twisted.mail.smtp.ESMTPClient
is no longer an functional attribute - the usage of secure transport doesn't depend on the value anymore. It's time to remove it from the class.
Attachments (9)
Change History (32)
Changed 10 years ago by
Attachment: | 5852.patch added |
---|
comment:1 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | argonemyth deleted |
comment:2 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks for working on this.
This patch doesn't accomplish the desired result, which is to emit a deprecation when the tlsMode
attribute is read or written. The goal is to warn developers with code that manipulates this attribute so they know the attribute no longer means anything. Introducing a new API that emits a deprecation warning doesn't accomplish the goal, since existing code won't use that API.
A property can be used to achieve the desired effect for when tlsMode
is read. Unfortunately, since it's on a classic class, write properties don't work, so a __setattr__
hook may be necessary.
Changed 10 years ago by
Attachment: | 5852.2.patch added |
---|
comment:3 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
comment:4 Changed 10 years ago by
Cc: | Nam Nguyen added |
---|
Line 1246 in smtp.py
should be reflowed to 79 columns.
Line 612 in test_smtp.py
looks redundant. I think line 1529 in that same file is also not needed. Line 1542 in the same file should be reflowed.
I would replace is
with has been
in 5852.removal
.
Changed 10 years ago by
Attachment: | 5852.3.patch added |
---|
comment:5 Changed 10 years ago by
Cc: | moijes12 added |
---|
comment:6 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks for your continuing work on this issue.
warnings.catch_warnings
is not how we unit test deprecations. Please stick to the convention established in other deprecation unit tests in Twisted.- The implementation of
__setattr__
breaks all attribute setting, completely. - The news fragment can just say that the attribute is deprecated, the version information is not needed (the news fragment will appear in the news file under a section for a particular version of Twisted).
Changed 10 years ago by
Attachment: | 5852.4.patch added |
---|
comment:7 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
comment:8 Changed 10 years ago by
twisted.mail.smtp.ESMTPClient being a classic class, I couldn't use descriptors. So I had to use setattr hook. What I did notice was that the test messages were getting suppressed because of previous function definitions and therefore I had to reset the C{.suppress} . Atleast that is what my understanding of it is.
comment:9 Changed 10 years ago by
Keywords: | review removed |
---|---|
Owner: | set to moijes12 |
Thanks. A few issues remain:
- The new test asserts the wrong thing. Creating an ESMTPClient instance should not cause a deprecation warning, twisted.mail.smtp.ESMTPClient is deprecated since Twisted 12.2.
ESMTPClient
is not, in fact deprecated. Only thetlsMode
attribute is deprecated. This means:- The deprecation warning message needs to be changed
- The warning needs to be emitted only if the
tlsMode
attribute is set. Perhaps it is set as a side effect of instantiatingESMTPClient
now, but if so that is a problem that needs to be addressed. Using non-deprecated Twisted APIs must not trigger deprecation warnings.
- 12.2 has been released. The next release will probably be 12.3, so that's the version
tlsMode
needs to be deprecated in. - Pass an argument to the
flushWarnings
call. Otherwise you flush every single warning that has happened, and you avoid testing that thestacklevel
argument passed towarnings.warn
was given the correct value. The argument is a list of functions which could have triggered the warning. If thestacklevel
argument has the wrong value, then the wrong function will be marked as triggering the warning andflushWarnings
will tell you about this by not returning the warning, so your test can fail.
Thanks again.
Changed 10 years ago by
Attachment: | 5852.5.patch added |
---|
comment:10 Changed 10 years ago by
Keywords: | review added |
---|---|
Owner: | moijes12 deleted |
tlsMode being deprecated, I've removed the its initialisation from the init method of ESMTPClient. The messages have been updated.
Changed 10 years ago by
Attachment: | 5852.6.patch added |
---|
comment:12 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to lvh |
The bug exarkun mentioned is now closed. The patch looks okay to me, apart from the obvious fact that 12.3 is released now, so it should be deprecated in 13.0. I'll fix that and resubmit for review.
comment:13 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | lvh deleted |
New patch added, please review.
comment:14 Changed 9 years ago by
Cc: | Richard Wall added |
---|---|
Keywords: | review removed |
Owner: | set to Richard Wall |
Status: | new → assigned |
Code Review
- Patch didn't apply cleanly to trunk for me - probably since #3989.
$ patch -p0 < ../5852.7.patch patching file twisted/mail/smtp.py Hunk #4 FAILED at 1247. 1 out of 4 hunks FAILED -- saving rejects to file twisted/mail/smtp.py.rej patching file twisted/mail/test/test_smtp.py Hunk #1 FAILED at 6. Hunk #2 succeeded at 611 (offset 2 lines). Hunk #3 FAILED at 1518. 2 out of 3 hunks FAILED -- saving rejects to file twisted/mail/test/test_smtp.py.rej patching file twisted/mail/topfiles/5852.removal
- There's some trailing Whitespace in the patch.
- There are still lines > 80 chars
- Remove the word "Test " from test method docstring - instead just "ESMTPClient.tlsMode is deprecated"
- This ticket title is quite misleading
I'll clean up the patch and resubmit.
Changed 9 years ago by
Attachment: | 5852.8.patch added |
---|
comment:15 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Status: | assigned → new |
Ready for review in attachment:5852.8.patch
- Patch should now merge cleanly to trunk
- Re-flowed long lines
- Moved deprecation test to alongside existing ESMTPClient TLS tests.
- Removed unused warning module in test_smtp.py
- Removed the "suppress" attribute from the test method - it looked unnecessary.
- Added epytext ref to the test docstring.
-RichardW.
comment:16 follow-up: 17 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
This only emits a deprecation warning when writing to tlsMode
, not when reading. Both should be handled. Otherwise, this looks good.
Changed 9 years ago by
Attachment: | 5852.9.patch added |
---|
Now with a getattr method as suggested by tomprince
comment:17 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Richard Wall deleted |
Replying to tom.prince:
This only emits a deprecation warning when writing to
tlsMode
, not when reading. Both should be handled. Otherwise, this looks good.
Ok.
- I renamed the actual tlsMode value to _tlsMode
- I added a getattr method and modified setattr to access the private var
- ...and some new tests.
I didn't try and do anything about accessing or setting it as a class variable
-RichardW.
comment:18 Changed 9 years ago by
Owner: | set to Thijs Triemstra |
---|---|
Status: | new → assigned |
Putting this in a branch and see what the buildfarm thinks of it.
comment:19 Changed 9 years ago by
Author: | → thijs |
---|---|
Branch: | → branches/deprecate-tlsmode-5852 |
(In [37187]) Branching to 'deprecate-tlsmode-5852'
comment:21 Changed 9 years ago by
Author: | thijs → moijes12, rwall |
---|---|
Owner: | Thijs Triemstra deleted |
Status: | assigned → new |
Forced a build.
comment:22 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Richard Wall |
One minor point:
- The
AttributeError
should useself.__class__.__name__
instead of hardcoding it.
Otherwise, this looks good. Thanks for your work on this. Please commit after fixing that.
comment:23 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
Are you looking for something like this ?