Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-78319: add UTF8 marker per RFC #9436

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gordonmessmer
Copy link
Contributor

@gordonmessmer gordonmessmer commented Sep 20, 2018

This change implements RFC 6855 UTF8 APPEND per guidance from Sam Varshavchik:
https://bugs.python.org/issue34138

https://bugs.python.org/issue34138

@gordonmessmer gordonmessmer requested a review from a team as a code owner September 20, 2018 03:30
@the-knights-who-say-ni
Copy link

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.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@gordonmessmer
Copy link
Contributor Author

I thought I'd signed the CLA previously... I've signed it today. Please let me know what other steps I can take to help merge this patch.

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@arhadthedev arhadthedev changed the title bpo-34138: add UTF8 marker per RFC gh-78319: add UTF8 marker per RFC Feb 9, 2023
@arhadthedev arhadthedev added stdlib Python modules in the Lib dir topic-email labels Feb 9, 2023
@arhadthedev
Copy link
Member

@gordonmessmer tests are failing (see Details links under Some checks were not successful section). Would you mind to continue working on the PR, please?

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@gordonmessmer
Copy link
Contributor Author

I'll look at it again, yes. I'm getting odd results from updated tests, so some additional work is still needed...

@gordonmessmer
Copy link
Contributor Author

Tests look like they're passing, but trying this on a live IMAP server fails, because the server actually receives:

LLFC3 APPEND INBOX UTF8 (~{63}\r\n

... when Sam suggested that it should receive:

LLFC3 APPEND INBOX NIL NIL UTF8 (~{63}\r\n

But that's probably a larger bug in the imaplib module, and not directly related to this change.

@arhadthedev
Copy link
Member

@warsaw, @maxking (as active e-mail experts)

@gordonmessmer
Copy link
Contributor Author

I've asked Sam to chime in. The RFC calls those arguments optional. Based on its behavior, it seems that Courier expects them to be specified as NIL when they are not provided, rather than left out entirely.

@gordonmessmer
Copy link
Contributor Author

gordonmessmer commented Feb 14, 2023

I added a commit for consideration. Transforming None to "NIL" is RFC-compliant (which is consistent with the intent of this PR). Tests pass, and this works with Courier IMAP.

@arhadthedev arhadthedev requested review from warsaw and removed request for a team February 14, 2023 08:10
@gordonmessmer
Copy link
Contributor Author

The NIL requirement may be a bug in Courier (ironically also caused by UTF8 support), so the "imaplib: transform None into "NIL"" patch might not be needed.

Everything else looks good, I think.

@gordonmessmer
Copy link
Contributor Author

Sam released a new version of Courier, and I've verified that the None->NIL transformation is no longer necessary, so I've backed it out.

@gordonmessmer
Copy link
Contributor Author

Please let me know if there's anything I can do to help move this forward.

@svarshavchik svarshavchik mannequin mentioned this pull request Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review stdlib Python modules in the Lib dir topic-email
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants