gh-76578: Fix textwrap.wrap() so it's stable if run twice.#5615
gh-76578: Fix textwrap.wrap() so it's stable if run twice.#5615larryhastings wants to merge 2 commits intopython:mainfrom
Conversation
ned-deily
left a comment
ned-deily
left a comment
There was a problem hiding this comment.
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.
serhiy-storchaka
added
type-bug
serhiy-storchaka
left a comment
serhiy-storchaka
left a comment
There was a problem hiding this comment.
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.
I would write this as
cur_line[-1] = spacer
cur_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.
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.
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.
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.
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.
iritkatriel
left a comment
iritkatriel
left a comment
There was a problem hiding this comment.
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 a different direction (adding a new feature instead of considering this a bug). If so, shouldn't this PR (and its underlying issue) be closed/rejected? |
github-actions
Bot
removed
the
stale
No answer. Is this truly still a bug fix or is it now a feature request? |
erlend-aasland
changed the title
|
This PR is stale because it has been open for 30 days with no activity. |
|
All commit authors signed the Contributor License Agreement. |
github-actions
Bot
removed
the
stale
|
This PR is stale because it has been open for 30 days with no activity. |
10 participants
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 word would have
fit if that whitespace blob was only 1 character long.
https://bugs.python.org/issue32397