Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-76578: Fix textwrap.wrap() so it's stable if run twice.#5615
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
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.
This seems like a bug to me so there should be no issue in fixing it for 3.7 and probably should be OK for 3.6.x.
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 have few minor style suggestions, but in any case LGTM.
| spacer = " " | ||
| new_len = cur_len - len(cur_line[-1]) + len(spacer) + len(chunks[-1]) | ||
| if new_len <= width: | ||
| cur_line.pop() |
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 write this as
cur_line[-1]=spacercur_line.append(chunks.pop())
but this is a matter of style.
| and cur_line and cur_line[-1].strip() == ''): | ||
| spacer = " " | ||
| if (self.fix_sentence_endings | ||
| and (len(cur_line) > 1) |
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 think this would look better without parenthesis.
| # original is 31 characters long: | ||
| # 0 1 2 3 | ||
| # 1234567890123456789012345678901 | ||
| original = "xxxx xxxx xxxx xxxx xxxx. xxxx" |
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.
Could you please use more meaningful example? If the test will fail it more convenient to see different words in the report rather of repeated 'xxxx'.
| wrapped2 = wrap(wrapped) | ||
| self.assertEqual(wrapped, wrapped2) | ||
| def test_wrap_stability_with_fix_sentence_endings(self): |
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.
Seems this case is already covered by test_fix_sentence_endings. test_fix_sentence_endings fails if disable the correction for fix_sentence_endings.
@larryhastings What's the status of this PR? It looks like@serhiy-storchaka had a few minor review comments |
I removed the " needs backport to 3.6" label, the 3.6 branch no longer accept bugfixes (only security fixes are accepted):https://devguide.python.org/#status-of-python-branches |
| # This is all complicated slightly by fix_sentence_endings, | ||
| # where the chunk we add back in might need to be two spaces | ||
| # instead of one. | ||
| if (chunks and self.drop_whitespace |
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.
According to the doc, drop_whitespace means "whitespace at the beginning and ending of every line (after wrapping but before indenting) is dropped".
But when you do below
new_len = cur_len - len(cur_line[-1]) + len(spacer) + len(chunks[-1])
doesn't that amount to dropping whitespace within the line?
I see what you're trying to solve here, but I think that the definition of drop_whitespace, if I understand it correctly, makes wrap inherently non-idempotent.
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.
As per my comment, I think this might break drop_whitespace. Please check.
This PR is stale because it has been open for 30 days with no activity. |
It appears this change is going in adifferent direction (adding a new feature instead of considering this a bug). If so, shouldn't this PR (and its underlying issue) be closed/rejected? |
No answer. Is this truly still a bug fix or is it now a feature request? |
This PR is stale because it has been open for 30 days with no activity. |
python-cla-botbot commentedApr 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
This PR is stale because it has been open for 30 days with no activity. |
Uh oh!
There was an error while loading.Please reload this page.
Fix
textwrap.wrap()so it's stable. In certain fiddly circumstances,textwrap.wrap(x)wasn't the same astextwrap.wrap(textwrap.wrap(x)),which was surprising. This happened when a line was wrapped at a whitespace
blob that was longer than 1 character, but the following wordwould have
fit if that whitespace blob was only 1 character long.
https://bugs.python.org/issue32397