| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Why inlining _len(), I don't have seen performance issues and it's less readable (less pythonic)
Sorry, something went wrong.
There was a problem hiding this comment.
How do you know where to break once you have the whole value?
Sorry, something went wrong.
There was a problem hiding this comment.
Hello I was reading too fast. In my version there's the _wide boolean function.
So here width += _wide(char) + 1
Sorry, something went wrong.
There was a problem hiding this comment.
And _len is just return sum(2 if _wide(char) else 1 for char in text) with no performance issues
Sorry, something went wrong.
There was a problem hiding this comment.
More pythonic, DRY.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Why note use enumerate(), it's less readable (less pythonic)
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Interested in that, the code was strongly tested for txt2tags and don't catch this problem.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
"Explicit is better than implicit." but the more important is that both solutions are correct.
Sorry, something went wrong.
|
Don't see my author credit |
Sorry, something went wrong.
|
And you miss the if self.width <= 0: bug fixed in #89 |
Sorry, something went wrong.
Co-authored-by: Florent Gallaire <fgallaire@gmail.com>
You're right! And trying to split a wide character yield to an infinite loop.
Gladly fixed and co-authored you. |
Sorry, something went wrong.
|
Thanks @JulienPalard, I'm so happy ! I had almost lost hope to see this issue fixed. |
Sorry, something went wrong.
| 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)") |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Related to:
https://bugs.python.org/issue24665