Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

Fix miscalculation of cjk characters. #9

Merged
merged 2 commits into from
Aug 24, 2015

Conversation

bcho
Copy link
Contributor

@bcho bcho commented Aug 22, 2015

Patch is taken from #8, and the tests had been fixed.

If the column contains east asian unicode characters, such as "中文".
`len` function can't return the right width.
Returns:
String width.
"""
if isinstance(string, Color):
Copy link
Owner

Choose a reason for hiding this comment

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

Can you do this without isinstance? I don't like isinstance, it makes code inflexible and hard to use in other libraries. What if someone made something like colorclass where they subclass string/unicode? terminaltables wouldn't work with their library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Robpol86 Yes, isinstance should be avoid here, but can Color provide a interface just like the normal str? I mean, when iterating / decoding the Color instance, it should return the real value instead of the escaped one, I think this will make things much more easier.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it won't be so easy since something like sys.stdout.write() iterates the Color instance and that expects the color version. You're free to open an issue/pr in colorclass. Maybe instead you can just use hasattr() or getattr() for value_no_colors?

@bcho bcho force-pushed the fix/cjk-width branch 2 times, most recently from 79f4c4c to 9bfaf92 Compare August 24, 2015 01:59
@bcho
Copy link
Contributor Author

bcho commented Aug 24, 2015

@Robpol86 updated, and I made a rebase for these changes.

@Robpol86
Copy link
Owner

Everything else looks good. Once you fix that one line I'll accept. Thanks!

@bcho
Copy link
Contributor Author

bcho commented Aug 24, 2015

@Robpol86 Fixed!

Robpol86 added a commit that referenced this pull request Aug 24, 2015
Fix miscalculation of cjk characters.
@Robpol86 Robpol86 merged commit 14b9dd4 into Robpol86:master Aug 24, 2015
@bcho bcho deleted the fix/cjk-width branch August 24, 2015 02:24
@Robpol86
Copy link
Owner

Robpol86 commented Sep 4, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants