Skip to content

Conversation

@zangjiucheng
Copy link
Contributor

@zangjiucheng zangjiucheng commented Jun 10, 2025

@zangjiucheng zangjiucheng changed the title gh-135307: Support email unlimited line lengths when policy max_line_length is 0 or None gh-135307: Fix email error when policy max_line_length is set to 0 or None Jun 10, 2025
@ZeroIntensity ZeroIntensity added topic-email needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jun 10, 2025
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Triage silliness

Comment on lines 1 to 2
:mod:`email`: Ensure policy accepts unlimited line lengths by
treating 0 or :const:`None` as :data:`sys.maxsize`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:mod:`email`: Ensure policy accepts unlimited line lengths by
treating 0 or :const:`None` as :data:`sys.maxsize`
:mod:`email`: Ensure policy accepts unlimited line lengths by
treating 0 or :const:`None` as :data:`sys.maxsize`.

Copy link
Member

Choose a reason for hiding this comment

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

I would phrase this as follows: "Fix exception in set_content when encoding text and max_line_length is set to 0 or None (unlimited)." maxsize is an implementation detail and not appropriate for a news item. And it's not the policy that isn't accepting the unlimited line lengths, it's set_content ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me, we can keep implementation detail behind :)

@zangjiucheng
Copy link
Contributor Author

Haha, nice catch lol

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Some comments. I'll review more on my laptop.

@zangjiucheng
Copy link
Contributor Author

@picnixz Thanks for your detailed review. I addressed all the issues you mentioned in the new commit.

@zangjiucheng zangjiucheng requested a review from picnixz June 19, 2025 14:50
if cte is None:
# Use heuristics to decide on the "best" encoding.
if max((len(x) for x in lines), default=0) <= policy.max_line_length:
if max((len(x) for x in lines), default=0) <= maxlen:
Copy link
Member

Choose a reason for hiding this comment

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

We can use: max(map(len, lines), default=0), though I'm not sure which is the fastest and which is the most "readable".

@bitdancer What is the style you'd recommend in general in the email package? would you rather for readability, succintness, or performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just my comment: we're not required to take the fastest route here, since we may not have a heavy load on the email module. Still wish to hear more comments before we resolve this :)

Copy link
Member

Choose a reason for hiding this comment

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

Well, mailman can put a pretty heavy load on the email package, and it is one of the biggest consumers of the package. While I know there are opportunities for performance improvements in the new email code that I haven't gotten around to investigating, I personally tend to do what others call micro-optimizations when I notice them[*], unless they make the code really hard to understand. Not that I know which is faster, here.

[*] I read an article once where Knuth felt his comment had been misinterpreted, and pointed out that if you routinely ignore optimization opportunities you pretty quickly end up with a really slow program.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[*] I read an article once where Knuth felt his comment had been misinterpreted, and pointed out that if you routinely ignore optimization opportunities, you pretty quickly end up with a really slow program.

@bitdancer Thanks for sharing the additional context — it’s really helpful to know there’s a concrete performance impact for real users like Mailman.

I agree that starting with small, focused micro-optimizations makes sense, especially since we’re planning a broader optimization pass for the email module.

I’ve incorporated your suggestion and updated the comment in the code accordingly.
Please also let me know if you have any specific areas you think would benefit most from further optimization work. Hope to collaborate on more work with you!

Copy link
Member

Choose a reason for hiding this comment

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

FTR, I don't think this will change much because we've seen times when code is slower with an map() and sometimes slower with a comprehension. If we want to make the code faster, we should run better benchmarks.

@zangjiucheng
Copy link
Contributor Author

Hi everyone, sorry to bother you. Is there any update on this PR?

@bitdancer
Copy link
Member

I couldn't figure out how to get review mode to show me all the conversations, so I made individual comments in conversation mode. Sorry if that makes more work for others, or if I ended up with misplaced comments.

@zangjiucheng
Copy link
Contributor Author

@bitdancer No worries, I saw all your comments and I will address them one by one later today.

Copy link
Member

@bitdancer bitdancer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@bitdancer bitdancer merged commit 6d45cd8 into python:main Nov 2, 2025
50 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 2, 2025
…o 0 or None (pythonGH-135367)

(cherry picked from commit 6d45cd8)

Co-authored-by: Jiucheng(Oliver) <git.jiucheng@gmail.com>
RDM: Like the change made in a earlier PR to the folder, we can/must use 'maxlen' as a stand in for 'unlimited' when computing line lengths when max_line_length is 0 or None; otherwise the computation results in a traceback.
@miss-islington-app
Copy link

Sorry, @zangjiucheng and @bitdancer, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 6d45cd8dbb07ae020ec07f2c3375dd06e52377f6 3.13

@bedevere-app
Copy link

bedevere-app bot commented Nov 2, 2025

GH-140915 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Nov 2, 2025
zangjiucheng added a commit to zangjiucheng/cpython that referenced this pull request Nov 2, 2025
…s set to 0 or None (pythonGH-135367)

(cherry picked from commit 6d45cd8)

Co-authored-by: Jiucheng(Oliver) <git.jiucheng@gmail.com>
RDM: Like the change made in a earlier PR to the folder, we can/must use 'maxlen' as a stand in for 'unlimited' when computing line lengths when max_line_length is 0 or None; otherwise the computation results in a traceback.
@bedevere-app
Copy link

bedevere-app bot commented Nov 2, 2025

GH-140917 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 Nov 2, 2025
bitdancer pushed a commit that referenced this pull request Nov 2, 2025
…to 0 or None (GH-135367) (#140915)

gh-135307: Fix email error when policy max_line_length is set to 0 or None (GH-135367)
(cherry picked from commit 6d45cd8)


RDM: Like the change made in a earlier PR to the folder, we can/must use 'maxlen' as a stand in for 'unlimited' when computing line lengths when max_line_length is 0 or None; otherwise the computation results in a traceback.

Co-authored-by: Jiucheng(Oliver) <git.jiucheng@gmail.com>
bitdancer pushed a commit that referenced this pull request Nov 2, 2025
…to 0 or None (GH-135367) (#140917)

[3.13] gh-135307: Fix email error when policy max_line_length is set to 0 or None (GH-135367)
(cherry picked from commit 6d45cd8)

Co-authored-by: Jiucheng(Oliver) <git.jiucheng@gmail.com>
RDM: Like the change made in a earlier PR to the folder, we can/must use 'maxlen' as a stand in for 'unlimited' when computing line lengths when max_line_length is 0 or None; otherwise the computation results in a traceback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants