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

#22027: RFC 6531 (SMTPUTF8) support in smtplib

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

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


Total messages: 3
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
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
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 :)

File Doc/library/smtplib.rst (right):

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 :)

File Lib/smtplib.py (right):

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.

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
> uppercase here?
Nice catch, I think I meant to write optionlist instead of options there.

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.

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.

File Lib/test/test_smtplib.py (right):

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.

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.

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+