Attach files with filenames that include non-ASCII characters #69
Conversation
|
Unsure of why these tests are failing on Python3 -- looks to be a broader issue than just my tests. |
|
cc: @amcgregor, just want to bring this to your attention. |
There was a problem hiding this comment.
Howdy! Indeed, those Travis errors are me, not you, as I attempt to clean up the develop branch a bit. Thanks for the pull, especially with tests! There is the question of the change being breaking, however.
If this could be updated to either allow the old name, but with a DeprecationWarning emitted, or to preserve the original argument name, that'd be awesome.
| @@ -258,7 +258,8 @@ def mime(self): | |||
| return message | |||
|
|
|||
| def attach(self, name, data=None, maintype=None, subtype=None, | |||
| inline=False, filename=None, encoding=None): | |||
| inline=False, filename=None, filename_charset='', filename_language='', | |||
| content_encoding=None): | |||
There was a problem hiding this comment.
Unsure of why this needed to be a breaking change? (encoding → content_encoding) Why write more and needlessly break existing use?
There was a problem hiding this comment.
Oops! You're totally right. My goal was to clarify the difference between the encoding of the attachment and the encoding of the attachment name. However, RFC uses "charset" rather than "encoding" and I forgot to change it back. Guess this is what code review is for!
If that is your only concern, I'd be happy to push that change up right now. Otherwise, I'll bundle all of my changes into another commit for easier reviewing.
There was a problem hiding this comment.
That's pretty much the only thing stopping me from accepting the pull immediately. :) I'll squash when merging, so don't worry about having multiple individual commits.
| assert 'Farewell cruel world.' in unicode(message) | ||
| assert 'filename*="\'en-us\'%E2%98%83.txt"' in unicode(message) # ☃ is encoded in ASCII as \xe2\x98\x83, which is URL encoded as %E2%98%83 | ||
| assert 'dW5pY29kZSBzbm93bWFu' in unicode(message) # unicode snowman in base64 | ||
|
|
…sure backwards compatibility
|
Thanks again! |
|
Do you plan on releasing a bumped version on PyPi? Would make pulling this commit into my project a lot easier. Thanks! |
|
Aye; I'll try to tackle that (and fixing these tests…) for a release this week-end. |
|
Just curious if you had an update on this! Thanks! |
|
Yeah… spent a ludicrous amount of time this week-end wrangling with the tests. Things like the DNS package used previously is not Python 3 compatible, nor was the mock SMTP server. Migration to Pytest (from Nose) is also part of this. T'was a bit bigger of a job than two afternoons on the week-end could account for. I'll continue updating/fixing them as the week progresses and will re-try for a release next week-end. |
|
No worries, didn't realize it would be such a big deal! Thanks so much for slogging through it for a minor change. I do appreciate it. |
|
It never hurts to help. 😆 |
|
Tests pass on all supported platforms now. There's an interesting oddity between Python 2 and 3, where the encoded filename is quoted in Python 2, but not quoted in 3. I've verified that unquoted is acceptable according to the RFC, so no damage beyond extra conditionals in the tests. I've shoved the major test update (ripping out the DNS bits, etc.) into a branch and will roll a release including the changes/fixes so far on Saturday. Could you do me a huge favour and double-check that the current Thanks! |
|
@amcgregor Just seeing this now! Thank you so very much. I really appreciate your dedication to the issue; I know it was a major task getting the tests in order for such a small fix. |
amcgregor
added
2.enhancement
RFC 2231 allows us to specify a language and a charset for filenames that include non-ASCII characters. Python's
emailmodule already includes some nifty functionality to handle this. This PR simply exposes that functionality to users of marrow/mailer.