Skip to content

bpo-34800: Fix email.contentmanager raise error when policy.max_line_length is 0 or None #9578

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

bpo-34800: Fix email.contentmanager raise error when policy.max_line_length is 0 or None #9578

wants to merge 6 commits into from
+47 −2

Conversation

silane
Copy link

@silane silane commented Sep 25, 2018 •
edited by bedevere-bot
Loading

@silane silane requested a review from a team as a code owner September 25, 2018 20:11
@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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this 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!

@tirkarthi
Copy link
Member

Thanks for the PR. I would suggest adding some tests with None and 0 for max_line_length perhaps you can copy the code in the bug tracker and add them here. I think this requires a NEWS entry. You can refer to https://devguide.python.org/committing/#what-s-new-and-news-entries.

maxking
maxking reviewed May 28, 2019
View reviewed changes
Copy link
Contributor

@maxking maxking left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @silane !

I have added some inline comments.

Lib/email/quoprimime.py
@@ -168,6 +168,8 @@ def body_encode(body, maxlinelen=76, eol=NL):

"""

if not maxlinelen:
maxlinelen=float('+inf')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should perhaps be maxlinelen = sys.maxsize instead (along with one space each around =).

See https://bugs.python.org/issue33524 where float('+inf') can cause TypeError.

Misc/NEWS.d/next/Library/2018-09-26-15-18-18.bpo-34800.auD5YK.rst
@@ -0,0 +1,2 @@
Fix `email.contentmanager.set_content` raise error when
Copy link
Contributor

Choose a reason for hiding this comment

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

Some small suggestions:

Fixed a bug where ``email.contentmanager.set_content`` raised error when
``email.policy.Policy.max_line_length`` is 0 or ``None``. Patch by silane.

In rst, <code> sections needs to be wrapped in double backticks.

@nimish
Copy link

nimish commented Sep 20, 2019

Is this patch dead? It's very much needed when using email.policy.HTTP.

@csabella
Copy link
Contributor

@silane, please address the review comments. Thank you!

@gstein
Copy link

gstein commented Jun 21, 2020

I ran into this bug when I attempted to use email.policy.HTTP and set_bytes_content.

My workaround was to use email.policy.SMTP for encoding binary files in an HTTP multipart form. It's the same as policy.HTTP but for the max_line_length=None

@kumaraditya303
Copy link
Contributor

This PR is awaiting changes for two years and and has merge conflicts and the review comments were not addressed hence I am closing it, if you still want to work this can reopened or a new PR can be created.

@gstein
Copy link

gstein commented Apr 21, 2022

Let me get this straight:

This patch corrects problems in the library, but has been discarded because a few spaces were missing?

@kumaraditya303
Copy link
Contributor

See this comment #9578 (comment)

@gstein
Copy link

gstein commented Apr 21, 2022

You people are insane nowadays. "If I can't hit a button to merge, then I will not fix Python".

Whatever happened to "Apply the provided patch. Make a couple tweaks. Commit."

Today it is: "no matter how small a tweak is needed, I will not perform it. Even if that means leaving bugs within Python."

Back in the 1990s, I'd just fix the problem.

cc: @gvanrossum ... sad change of strategy, old friend.

@kumaraditya303
Copy link
Contributor

There is no issue in reopening a PR if the author is still working on it, I reopened this PR but if the comments of the maintainer will not be addressed then the PR will be stale.

@gstein
Copy link

gstein commented Apr 21, 2022

It is impossible for people to fix the original PR, when that person disappears. Thus, there are two possibilities:

  1. somebody who happens along (like myself) needs to construct a new PR incorporating the feedback
  2. "the maintainer" (fancy new way to silo python dev, and who happens to be responsible for this particular code) could make the change

What is best for Python?

Do you want me to construct a new PR with the whitespace changes, and the maxsize change? Is that the bar that you want to set for fixing Python?

I will do it. I just want you to state that this level of make-work is necessary.

@gvanrossum
Copy link
Member

@gstein There's no need to get so ornery about it. This probably just fell through the cracks because nobody cared enough.

gvanrossum and others added 2 commits April 23, 2022 08:51
@gvanrossum @maxking
Update Lib/test/test_email/test_contentmanager.py
c1d2d2d 
Co-authored-by: Abhilash Raj <maxking@users.noreply.github.com>
@gvanrossum @maxking
Update Lib/test/test_email/test_contentmanager.py
9beb4f5 
Co-authored-by: Abhilash Raj <maxking@users.noreply.github.com>
@gvanrossum
Copy link
Member

No need to make a new PR. Changes can be made via the GitHub ui. I just fixed the missing spaces for example.

@gstein
Copy link

gstein commented Apr 24, 2022

Note that, as a third party, I cannot make changes to this PR. That's why I suggested constructing a new PR to incorporate the feedback provided.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 24, 2022 •
edited
Loading

Note that, as a third party, I cannot make changes to this PR. That's why I suggested constructing a new PR to incorporate the feedback provided.

Note that @kumaraditya303 could not make any changes to the PR (or merge the PR) either, since he is a triager rather than a core dev. (I am also a triager rather than a core dev.)

@gvanrossum
Copy link
Member

Have you tried to make "suggestions"? A core dev could them apply those by clicking a button.

Since I don't use or understand the module being modified I'll unsubscribe now (Greg getting so upset didn't help).

@aaugustin
Copy link

Short term workaround (via monkey-patching) until this is fixed in Python:

email.policy.HTTP = email.policy.HTTP.clone(max_line_length=1_000_000_000)

@gstein
Copy link

gstein commented Aug 28, 2022 •
edited
Loading

Cool. Thanks, @aaugustin. I intend to create a new PR incorporating the above feedback, as something that the core devs can merge.

I'm not familiar with the "suggestion" feature that Guido referenced, but will look at that. I haven't looked at this for several months.

@silane silane mannequin mentioned this pull request Nov 23, 2023
Open
@crazymerlyn crazymerlyn mentioned this pull request Sep 1, 2024
Open
@github-actions GitHub Actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 27, 2025
@python-cla-bot
Copy link

The following commit authors need to sign the Contributor License Agreement:

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxking maxking maxking left review comments

@gvanrossum gvanrossum gvanrossum left review comments

Assignees
No one assigned
Labels
awaiting changes stale Stale PR or inactive for long period of time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.