gh-135307: Fix email error when policy max_line_length is set to 0 or None#135367
gh-135307: Fix email error when policy max_line_length is set to 0 or None#135367bitdancer merged 11 commits intopython:mainfrom
Conversation
[pull] main from python:main
| :mod:`email`: Ensure policy accepts unlimited line lengths by | ||
| treating 0 or :const:`None` as :data:`sys.maxsize` |
There was a problem hiding this comment.
| :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`. |
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
Makes sense to me, we can keep implementation detail behind :)
|
Haha, nice catch lol |
picnixz
left a comment
There was a problem hiding this comment.
Some comments. I'll review more on my laptop.
Misc/NEWS.d/next/Library/2025-06-10-18-02-29.gh-issue-135307.fXGrcK.rst
Outdated
Show resolved
Hide resolved
|
@picnixz Thanks for your detailed review. I addressed all the issues you mentioned in the new commit. |
Lib/email/contentmanager.py
Outdated
| 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
[*] 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!
There was a problem hiding this comment.
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.
|
Hi everyone, sorry to bother you. Is there any update on this PR? |
|
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. |
|
@bitdancer No worries, I saw all your comments and I will address them one by one later today. |
|
Thanks @zangjiucheng for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…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.
|
Sorry, @zangjiucheng and @bitdancer, I could not cleanly backport this to |
|
GH-140915 is a backport of this pull request to the 3.14 branch. |
…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.
|
GH-140917 is a backport of this pull request to the 3.13 branch. |
…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>
…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.
…o 0 or None (python#135367) 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.
email.policy.Policy.max_line_lengthis falsey #135307