bpo-28879 : add date if missing in smtplib.send_message #2655
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
@lafe3402, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bitdancer, @warsaw and @birkenfeld to be potential reviewers. |
|
oh wow... this is really a nice setup you've got running there! I see that some check failed for some trailing whitespace in my files... Unfortunatly, I seems to be unable to look at the log anymore :S. Anyway, how could I see it again? |
|
Ok got to find how to see it again. Now to understand what "trailing whitespace" is supposed to be for python and for... C (but I didn't touch the C part...) |
|
The clue is at this line in travis log: (https://travis-ci.org/python/cpython/jobs/252242241#L494) There is an extra space after the word |
Lib/test/test_smtplib.py
Outdated
| self.assertRegex(debugout, Date) | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
This only need two blankline to spacing class and class
Lib/smtplib.py
Outdated
| if msg.get('Date',None) is None: | ||
| msg['Date'] = email.utils.formatdate() | ||
|
|
||
|
|
There was a problem hiding this comment.
This only need one blankline.
Fix the whitespace and spacing
|
@lafe3402 CI will automatically re-trigger when you push your commit to GitHub. Also, I would recommend you to read this article: How to write a git commit message. This article is very useful that it tells how to write a good commit message. For example, your second commit message would be much better to changed to |
|
yeah... I though that I would have to make another PR request... so I've put what I've done in the subsection of the commit (it's in the expand the commit message if you expand it...). Thanks a lot for the feedback :) you're really pro-active on the comment and I appreciate that. Anything else I could do to ease the process? Regards, |
Lib/smtplib.py
Outdated
| @@ -928,6 +928,11 @@ def send_message(self, msg, from_addr=None, to_addrs=None, | |||
| header_prefix = 'Resent-' | |||
| else: | |||
| raise ValueError("message has more than one 'Resent-' header block") | |||
|
|
|||
| # RFC 5322 section 3.6, 4th Paragraph | |||
| if msg.get('Date',None) is None: | |||
There was a problem hiding this comment.
Here missing a whitespace after the ,.
| m['CC'] = 'Sally, Fred' | ||
| m['Bcc'] = 'John Root <root@localhost>, "Dinsdale" <warped@silly.walks.com>' | ||
| smtp = smtplib.SMTP(HOST, self.port, local_hostname='localhost', timeout=3) | ||
| smtp.send_message(m) |
There was a problem hiding this comment.
Maybe we can mock up the email.utils.formatdate() to ensure we have the same result to tests.
Lib/test/test_smtplib.py
Outdated
| mexpect = '%s%s\n%s' % (MSG_BEGIN, m.as_string(), MSG_END) | ||
| self.assertEqual(self.output.getvalue(), mexpect) | ||
| debugout = smtpd.DEBUGSTREAM.getvalue() | ||
| current_moment = email.utils.formatdate() |
There was a problem hiding this comment.
see above to mock this up.
There was a problem hiding this comment.
Thanks for the review :) I'll work on it tonight, my wife won't be home so I'll have nothing better to do :).
I'm not sure what you mean though on the "mock this up" but I guess you mean to use @mock(email.utils.formatdate) ?
I'll have to read to remember how to do it correctly if it's what you meant.
Following the discussion on the bpo-28879, I've put space between commas and the variable to be compliant and mocked the "Date" call being made to the send_message function
|
I'm sorry about this last re-commit, I can't run the test right now so I'm using the CI :$. |
|
@lafe3402 I can't pass the unittest on my laptop, too. You will need to check about the failed problem. |
|
@mlouielu , thanks for your patience, I had trouble finding some free time. |
|
Hi @mlouielu, Would you have some feedback? Just curious to see if there's something better I could've had done. Regards, |
|
Could you let me know what to do to accelerate the closure of this ticket? |
|
Find me some free time somewhere? I'm sorry, I just don't have the cycles to look at this, nor will I any time soon :( :( |
|
@bitdancer, Anything I could do to help you? I've been re-reading my question and I just don't want it to be misinterpreted as a "I want my request to get in!" kind of request. I'm genuinely trying to help you guys. I love python and I would love to contribute more. Also, David, are you the only one who can approve? If yes, should I learn email modules and try to alleviate some of the pain? Again, sorry for asking here, but I would like more meat on how to contribute or how to learn what I need to contribute XD. Regards, |
|
closing to re-open on a rebased project |
Hi I've tried to change the patch submitted on bpo-28879 to be on a github PR.
It's my first time contributing through PR, please let me know what I did wrong.
Regards,
Eric Lafontaine
https://bugs.python.org/issue28879