Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2)

#22027: RFC 6531 (SMTPUTF8) support in smtplib

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by milan.py
Modified:
4 years, 10 months ago
Reviewers:
berker.peksag, rdmurray
CC:
barry, AntoinePitrou, r.david.murray, jesstess, devnull_psf.upfronthosting.co.za, maciej.szulik, zvyn
Visibility:
Public.

Patch Set 1 #

Total comments: 1

Patch Set 2 #

Patch Set 3 #

Total comments: 17

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/smtplib.rst View 1 2 3 4 7 chunks +28 lines, -1 line 0 comments Download
Lib/smtplib.py View 1 2 3 4 9 chunks +36 lines, -6 lines 0 comments Download
Lib/test/test_smtplib.py View 1 2 3 4 5 chunks +134 lines, -13 lines 0 comments Download

Messages

Total messages: 3
berkerpeksag
http://bugs.python.org/review/22027/diff/12433/Doc/library/smtplib.rst File Doc/library/smtplib.rst (right): http://bugs.python.org/review/22027/diff/12433/Doc/library/smtplib.rst#newcode175 Doc/library/smtplib.rst:175: The :exc:`SMTPNotSupportedError` exception. You could move this to the ...
5 years, 8 months ago #1
r.david.murray
http://bugs.python.org/review/22027/diff/12668/Doc/library/smtplib.rst File Doc/library/smtplib.rst (right): http://bugs.python.org/review/22027/diff/12668/Doc/library/smtplib.rst#newcode298 Doc/library/smtplib.rst:298: No suitable authentication method was found. This is actually ...
5 years, 7 months ago #2
zvyn
5 years, 7 months ago #3
Thanks for the reviews! I added some comments, but the tl;dr is: I mostly agree
and am going to fix the patch now :)

http://bugs.python.org/review/22027/diff/12668/Doc/library/smtplib.rst
File Doc/library/smtplib.rst (right):

http://bugs.python.org/review/22027/diff/12668/Doc/library/smtplib.rst#newcod...
Doc/library/smtplib.rst:298: No suitable authentication method was found.
On 2014/08/10 00:02:25, r.david.murray wrote:
> This is actually still SMTPException, and should remain so, I think.  But you
> need to add SMTPNotSupportedError for the 'does not support AUTH' error.

The definition I made up includes options not supported by the server and would
fit here as 'PLAIN' would be an option to the 'AUTH' command and the error is
raised after all options were tested. But that should not be a blocker  so I'll
just change it :)

http://bugs.python.org/review/22027/diff/12668/Lib/smtplib.py
File Lib/smtplib.py (right):

http://bugs.python.org/review/22027/diff/12668/Lib/smtplib.py#newcode445
Lib/smtplib.py:445: resp = self.ehlo_resp.decode("utf-8").split('\n')
On 2014/08/10 00:02:25, r.david.murray wrote:
> This change is incorrect.  If the foreign server sends data that is not valid
> utf-8, we'd get an exception here.  'latin-1' is used so that *anything* the
> other server sends will be a accepted, with a decent chance of being able to
> parse the ascii parts correctly.
Agreed.

http://bugs.python.org/review/22027/diff/12668/Lib/smtplib.py#newcode511
Lib/smtplib.py:511: if 'SMTPUTF8' in options and self.does_esmtp:
On 2014/08/10 00:02:25, r.david.murray wrote:
> You called upper on what went into optionlist, but you are checking options
via
> uppercase here?
Nice catch, I think I meant to write optionlist instead of options there.

http://bugs.python.org/review/22027/diff/12668/Lib/smtplib.py#newcode661
Lib/smtplib.py:661: raise SMTPNotSupportedError(
On 2014/08/10 00:02:25, r.david.murray wrote:
> Looks like this should be added to the exception list in the doc string.
Jepp.

http://bugs.python.org/review/22027/diff/12668/Lib/smtplib.py#newcode816
Lib/smtplib.py:816: msg = _fix_eols(msg).encode(self.encoding)
On 2014/08/10 00:02:25, r.david.murray wrote:
> There is now a fair number of lines of code duplication between this and the
> MAIL command.  Perhaps it it time to factor it out.
*nods*

http://bugs.python.org/review/22027/diff/12668/Lib/test/test_smtplib.py
File Lib/test/test_smtplib.py (right):

http://bugs.python.org/review/22027/diff/12668/Lib/test/test_smtplib.py#newco...
Lib/test/test_smtplib.py:317: self.assertEqual(self.output.getvalue(), mexpect)
On 2014/08/10 00:02:25, r.david.murray wrote:
> Duplicated code block.
Oops

http://bugs.python.org/review/22027/diff/12668/Lib/test/test_smtplib.py#newco...
Lib/test/test_smtplib.py:638: self.enable_SMTPUTF8 = True
On 2014/08/10 00:02:25, r.david.murray wrote:
> Now that the smtpd patch has been committed, can this be simplified?
I assume that with simplify you mean using the kwarg in the setUp methods
instead of setting the instance variable here.

http://bugs.python.org/review/22027/diff/12668/Lib/test/test_smtplib.py#newco...
Lib/test/test_smtplib.py:851: smtp.close()
On 2014/08/10 00:02:25, r.david.murray wrote:
> What happens if you specify SMTPUTF8 but don't specify BODY=8BITMIME?
> 
> Also, these tests use sendmail, what about tests for when MAIL and DATA are
> called directly?
I'll add tests for that.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+