FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-24665: Add CJK support in textwrap by default. by JulienPalard · Pull Request #5649 · python/cpython · GitHub

Repository navigation

bpo-24665: Add CJK support in textwrap by default. - #5649

Closed
JulienPalard wants to merge 1 commit into
python:masterfrom
JulienPalard:textwrap-cjk
Closed

JulienPalard wants to merge 1 commit into
python:masterfrom
JulienPalard:textwrap-cjk

Conversation

Comment thread Lib/textwrap.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why inlining _len(), I don't have seen performance issues and it's less readable (less pythonic)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

How do you know where to break once you have the whole value?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hello I was reading too fast. In my version there's the _wide boolean function.
So here width += _wide(char) + 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

And _len is just return sum(2 if _wide(char) else 1 for char in text) with no performance issues

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

More pythonic, DRY.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I won't bet on the performances, calling _len from _slice adds two functions calls per character (one to _len and one to sum). In one case I'm doing it on a character, and in the other case in a whole string. Yes I could also factorize this ternary to a third function, but I don't find it more readable.

Comment thread Lib/textwrap.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why note use enumerate(), it's less readable (less pythonic)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Because it does not works with enumerate as the last incrementation were not done. I do not remember which case exactly but if you run the unit test you'll spot it easily, it was failing, I'll do if needed but can't right now.

fgallaire Feb 15, 2018 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Interested in that, the code was strongly tested for txt2tags and don't catch this problem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Your initial implementation was working thanks to your if cjk_len(text) <= index: return text, '' fixing the special case explicitly, I may have tried to avoid it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"Explicit is better than implicit." but the more important is that both solutions are correct.

Copy link
Copy Markdown

Don't see my author credit

Copy link
Copy Markdown

And you miss the if self.width <= 0: bug fixed in #89

JulienPalard force-pushed the textwrap-cjk branch 2 times, most recently from 45fd84d to 4623375 Compare March 6, 2018 22:42
Co-authored-by: Florent Gallaire <fgallaire@gmail.com>

Copy link
Copy Markdown
Member Author

And you miss the if self.width <= 0: bug fixed in #89

You're right! And trying to split a wide character yield to an infinite loop.

Don't see my author credit

Gladly fixed and co-authored you.

Copy link
Copy Markdown

Thanks @JulienPalard, I'm so happy ! I had almost lost hope to see this issue fixed.

Comment thread Lib/textwrap.py
if self.width <= 0:
raise ValueError("invalid width %r (must be > 0)" % self.width)
elif self.width == 1 and _width(text) > len(text):
raise ValueError("invalid width 1 (must be > 1 when CJK chars)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have done a more complex solution:

elif self.width == 1 and (sum(self._width(chunk) for chunk in chunks) >
                              sum(len(chunk) for chunk in chunks)):

It throws the exception earlier, but it's probably not absolutely necessary.

JulienPalard requested a review from vstinner March 28, 2018 21:16

terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The change I request is that this be closed because it is conceptually wrong. Textwrap works in terms of abstract 'characters' (codepoint), not physical units. I will explain this on the issue.

Aside from that, 2 is the wrong number to add, as 'double-width' characters are not actually twice as wide as fixed-pitch Ascii chars of the same height. See the issue for this as well.

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

methane closed this Jul 11, 2018
JulienPalard deleted the textwrap-cjk branch June 16, 2019 14:07
fgallaire mannequin mentioned this pull request Apr 10, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL