-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-135307: Fix email error when policy max_line_length is set to 0 or None #135367
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
Conversation
[pull] main from python:main
ZeroIntensity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Triage silliness
| :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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| :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.
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 ;)
There was a problem hiding this comment.
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 :)
|
Haha, nice catch lol |
picnixz
left a comment
There was a problem hiding this 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.
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.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
|
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. |
bitdancer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
|
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.
email.policy.Policy.max_line_lengthis falsey #135307