bpo-34800: Fix email.contentmanager raise error when policy.max_line_length is 0 or None by silane · Pull Request #9578 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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!
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.
| @@ -168,6 +168,8 @@ def body_encode(body, maxlinelen=76, eol=NL): |
|---|
| """ |
| if not maxlinelen: |
| maxlinelen=float('+inf') |
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.
| @@ -0,0 +1,2 @@ |
|---|
| Fix `email.contentmanager.set_content` raise error when |
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.
Is this patch dead? It's very much needed when using email.policy.HTTP.
@silane, please address the review comments. Thank you!
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
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.
Let me get this straight:
This patch corrects problems in the library, but has been discarded because a few spaces were missing?
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.
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.
It is impossible for people to fix the original PR, when that person disappears. Thus, there are two possibilities:
- somebody who happens along (like myself) needs to construct a new PR incorporating the feedback
- "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.
@gstein There's no need to get so ornery about it. This probably just fell through the cracks because nobody cared enough.
Co-authored-by: Abhilash Raj maxking@users.noreply.github.com
Co-authored-by: Abhilash Raj maxking@users.noreply.github.com
No need to make a new PR. Changes can be made via the GitHub ui. I just fixed the missing spaces for example.
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, 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.)
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).
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)
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.
This PR is stale because it has been open for 30 days with no activity.
