Fix stacktrace in twisted.smtp on buildbot #816
Conversation
Codecov Report
@@ Coverage Diff @@
## trunk #816 +/- ##
==========================================
- Coverage 91.59% 90.01% -1.58%
==========================================
Files 840 840
Lines 146766 146774 +8
Branches 12854 12854
==========================================
- Hits 134426 132117 -2309
- Misses 10098 12288 +2190
- Partials 2242 2369 +127 |
| @@ -0,0 +1 @@ | |||
| Sending a list of recipients with twisted.smtp.SenderFactory has been fixed. This fixes a problem found when running buildbot. | |||
There was a problem hiding this comment.
This fragment doesn't follow the style guidelines.
There was a problem hiding this comment.
Where is tardyp's review? The convention is for the review to be left on the PR. Failing that, the trac ticket. I don't see it in either place. Am I missing it somewhere?
There was a problem hiding this comment.
We did discuss that over IRC
|
|
||
| def test_SMTPClientRecipientUnicode(self): | ||
| """ | ||
| Use a L{unicode} recipient. |
| 'source@address', u'recipient@address', | ||
| BytesIO(b"Message body"), onDone, | ||
| retries=0, timeout=0.5) | ||
| return self._timeoutTest(onDone, clientFactory) |
There was a problem hiding this comment.
How about factoring the duplication between this and the previous test out? This will make it clear what's varying.
| """ | ||
| onDone = defer.Deferred() | ||
| clientFactory = smtp.SMTPSenderFactory( | ||
| 'source@address', (u'recipient1@address', b'recipient2@address'), |
There was a problem hiding this comment.
Oh wow what an awful API. This should probably be deprecated and removed.
| @@ -1896,7 +1897,7 @@ def __init__(self, fromEmail, toEmail, file, deferred, retries=5, | |||
| if not isinstance(_email, bytes): | |||
| _email = _email.encode('ascii') | |||
|
|
|||
| toEmailFinal.append(email) | |||
| toEmailFinal.append(_email) | |||
There was a problem hiding this comment.
Maybe it's not a good idea to have variables named email and _email in the same function?
|
It's important to go through all the parts of the review process as documented on https://twistedmatrix.com/trac/wiki/ReviewProcess#Reviewers:Howtoreviewachange. In this case, the parts that were skipped are:
These are pretty critical pieces of the process. We should improve the clarity of the language here - the comment in question is assumed to have to be either on the PR or the trac ticket, comments elsewhere (not in email, and especially not on IRC, where they'll be entirely lost to other interested readers) don't count. But in any case, the Since you, the author ( @rodrigc ) and not the reviewer ( @tardyp ) , are the project committer in this case, as per https://twistedmatrix.com/trac/wiki/ReviewProcess#Whocanreview, "the committer may accept it and land their change, but they are then responsible for the adequacy of the review". This review policy is in place to allow new reviewers to become familiar with our process so they can become committers - it is both out of process and unhelpful to that reviewer if they aren't actually following it. Usually if you have a non-committer review your changes, you should provide some feedback on the quality of the review in order to help give them confidence to eventually join the project. Hopefully this explanation makes it a little clearer why these aspects of the process are important, and what exactly is required. |
|
Well, I cannot say I did an official review, as I have no familiarity with that code. It looks like @rodrigc was overzealous merging that fix thinking that 17.5.0 was breaking Buildbot email feature. I think he can followup with another PR addressing the review comments. After a more thorough review, I doubt that this is recent regression, as the problem comes from e73c9c5 which is 10 month old. |
https://twistedmatrix.com/trac/ticket/9180