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.

Sign up for GitHub

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

Jump to bottom

gh-121542: Document trailing newline behavior in set_content() #121543

Merged
merged 7 commits into from
Jan 18, 2025
Merged

gh-121542: Document trailing newline behavior in set_content() #121543

merged 7 commits into from
Jan 18, 2025

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Jul 9, 2024 •
edited by github-actions bot
Loading

@ZeroIntensity ZeroIntensity requested a review from a team as a code owner July 9, 2024 16:47
@bedevere-app bedevere-app bot mentioned this pull request Jul 9, 2024
Closed
@bedevere-app bedevere-app bot added docs Documentation in the Doc dir skip news awaiting review labels Jul 9, 2024
@Eclips4 Eclips4 added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jul 9, 2024
Eclips4
Eclips4 approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@rapidcow
Copy link
Contributor

This behavior is only present in the default content manager, raw_content_manager. EmailMessage.set_content() is only a hook that delegate calls to the current policy's content manager, so we should either specify that or document this behavior only in email.contentmanager.rst.

Moreover, I'd suggest explicitly clarifying that such normalization applies only to str instances. If I were to use the following equivalent call (for illustrative purposes only):

msg.set_content(b'hello', 'text', 'plain', cte='quoted-printable', params={'charset': 'utf-8'})

then the binary string would be encoded exactly.

@ZeroIntensity ZeroIntensity mentioned this pull request Jul 10, 2024
Closed
@ZeroIntensity
Copy link
Member Author

Thanks to @rapidcow for the suggestion!

@rapidcow
Re-phrase docs of set_content behavior for str payloads
29cb612 
The previous patch only illustrates this behavior with a
very specific edge case, which IMO did not do a good job
elucidating the present ambiguous documentation, if not
worsening it by obscuring the true intention behind EOL
canonicalization for messages of text/* types.

This instead should address the more general case, abeit less
concrete, which seems like a fair compromise, considering
users who care about accurate representation of line endings
will likely carefully study the documentation and be assured
that this is an expected deviation from the legacy API, while
those who do not are not befuddled by a footnote that seems
to warn of something out of the blue.  I do not know if this
is true to the original author's intention, but it is my
personal understanding of this implementation at least. :/

The case of bytes is also addressed briefly in a subsequent
bullet point to assure the user that it is to be expected that
all bytes are preserved as any arbitrary binary file ought to
be.  I wish I could state this more explicitly, but that seems
rather hard without presumably restructuring this particular
page, so I will leave it to the original author... :)

Signed-off-by: Yizheng Meng <[email protected]>
@rapidcow
Copy link
Contributor

Actually, I stared at this for a while - I still think it could have been written better. It feels out of place when the rest of the body describing set_content() describes the behavior of specific arguments, while it is not so apparent which argument this piece of note is meant for. Also, as a note, it should probably be in a .. note:: directive, like the one about binary CTE right above.

This time I wrote up a patch for what I have in mind though. I pushed it to GitHub in this fork I made of your fork. Would it be fine if I made a PR of your PR so you may perhaps consider merging in my patch, @ZeroIntensity?

@ZeroIntensity
Copy link
Member Author

@rapidcow Sure, go ahead

@rapidcow rapidcow mentioned this pull request Jul 18, 2024
Merged
@rapidcow
Copy link
Contributor

rapidcow commented Jul 24, 2024 via email

@ZeroIntensity
Copy link
Member Author

Okay! Can you check the pull request sometime?
I think I made it a while ago....

Sorry, GH didn't notify me for whatever reason.

@cpython-cla-bot CPython CLA Bot
Copy link

cpython-cla-bot bot commented Jul 24, 2024 •
edited
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@ZeroIntensity
Copy link
Member Author

I forgot about this PR. @Eclips4 do you want to merge it, or should we get a second reviewer?

@Eclips4 Eclips4 self-requested a review January 14, 2025 15:35
@Eclips4 Eclips4 merged commit fba475a into python:main Jan 18, 2025
25 checks passed
@miss-islington-app
Copy link

Thanks @ZeroIntensity for the PR, and @Eclips4 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 18, 2025
@ZeroIntensity @rapidcow @miss-islington
pythongh-121542: Document trailing newline behavior in set_content() (
d16beb2 
pythonGH-121543)

(cherry picked from commit fba475a)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Yizheng Meng <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 18, 2025
@ZeroIntensity @rapidcow @miss-islington
pythongh-121542: Document trailing newline behavior in set_content() (
dd47421 
pythonGH-121543)

(cherry picked from commit fba475a)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Yizheng Meng <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

GH-128995 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 18, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jan 18, 2025

GH-128996 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jan 18, 2025
@ZeroIntensity ZeroIntensity deleted the patch-1 branch January 18, 2025 18:39
Eclips4 pushed a commit that referenced this pull request Jan 18, 2025
@miss-islington @ZeroIntensity @rapidcow
[3.13] gh-121542: Document trailing newline behavior in `set_content(…
b7ddcc3 
…)` (GH-121543) (#128995)

gh-121542: Document trailing newline behavior in `set_content()` (GH-121543)
(cherry picked from commit fba475a)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Yizheng Meng <[email protected]>
Eclips4 pushed a commit that referenced this pull request Jan 18, 2025
@miss-islington @ZeroIntensity @rapidcow
[3.12] gh-121542: Document trailing newline behavior in `set_content(…
104d37a 
…)` (GH-121543) (#128996)

gh-121542: Document trailing newline behavior in `set_content()` (GH-121543)
(cherry picked from commit fba475a)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Yizheng Meng <[email protected]>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 21, 2025
@ZeroIntensity @rapidcow @srinivasreddy
pythongh-121542: Document trailing newline behavior in set_content() (
eee05c5 
python#121543)

Co-authored-by: Yizheng Meng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eclips4 Eclips4 Awaiting requested review from Eclips4

Assignees
No one assigned
Labels
docs Documentation in the Doc dir skip news
Projects
Docs PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ZeroIntensity @rapidcow @Eclips4