Skip to content

fix: raise TypeError for empty/multi-char fillchar in ljust/rjust/center#229

Closed
koteshyelamati wants to merge 1 commit into
jquast:masterfrom
koteshyelamati:patch-1
Closed

fix: raise TypeError for empty/multi-char fillchar in ljust/rjust/center#229
koteshyelamati wants to merge 1 commit into
jquast:masterfrom
koteshyelamati:patch-1

Conversation

@koteshyelamati

Copy link
Copy Markdown

Bug

ljust(), rjust(), and center() silently accept a fillchar that is not exactly one character, producing incorrect output:

>>> wcwidth.ljust('hi', 5, '')   # should raise TypeError
'hi'                              # wrong! padding is missing
>>> wcwidth.ljust('hi', 5, 'ab') # should raise TypeError
'hiababab'                        # wrong! padding is doubled

Python's built-in equivalents raise TypeError: The fill character must be exactly one character long.

Fix

Added if len(fillchar) != 1: raise TypeError(...) at the top of each function, matching the built-in behavior.

Python's built-in str.ljust()/rjust()/center() raise TypeError when fillchar is not exactly one character. The wcwidth equivalents silently produce wrong output (empty padding for empty fillchar). Add the same validation.
Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():
Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():
Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():
Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if text.isascii() and text.isprintable():
if text.isascii() and text.isprintable():

Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if text.isascii() and text.isprintable():
if text.isascii() and text.isprintable():

Comment thread wcwidth/align.py
if text.isascii() and text.isprintable():
if len(fillchar) != 1:
raise TypeError('The fill character must be exactly one character long')
if text.isascii() and text.isprintable():

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if text.isascii() and text.isprintable():
if text.isascii() and text.isprintable():

@jquast

jquast commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Thanks for taking the time to submit.

I did consider the displayed width of fillchar should be narrow, the documentation is very clear about this:

Single character for padding (default space). Must have display width of 1 (not wide, not zero-width, not combining). Unicode characters like '·' are acceptable. The width is not validated.

(emphasis mine).

I decided not to validate the width of fillchar when I wrote this code. I did give it a lot of thought. I worked hard to make that clear in the docstring quoted above.

No algorithm could give any sensible result for a non-narrow fillchar. I do not fight against "glitch emojis" in my libraries, the overhead of checking for constraints of non-likely strings serves very little purpose, except maybe to more early inform a developer that they might have used the wrong variable or argument.


Further, this submission only checks the "length of the string", like standard python does, and this itself is a bug. To call it a "bug" that we do not match standard python in this way is a bit of a comedy.

This library solves the very problem python standard library fails to address -- that it should be checking for the displayed width of a string, and not the length or number of codepoints.

This emoji is narrow, and can be used as fillchar

>>> '\u265F\uFE0E'
>>> wcwidth.wcswidth('\u265F\uFE0E')
1
>>> wcwidth.ljust('hello', 10, fillchar='\u265F\uFE0E')
'hello♟︎♟︎♟︎♟︎♟︎'

But standard python, and your suggested code change has a bug, by using len it does not allow multi-codepoint strings or graphemes of narrow width to be used as fillchar:

>>> str.ljust('hello', 10, fillchar='\u265F\uFE0E')
Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    str.ljust('hello', 10, fillchar='\u265F\uFE0E')
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Second standard python bug, str.ljust allows "Wide" characters, such as CJK or Emoji so long as they are only a single codepoint, again, using the length of the string instead of the displayed width:

>>> str.ljust('hello', 10, '你')
'hello你你你你你'
>>> wcwidth.width('hello你你你你你')
15

Standard python is in error here, not wcwidth.

wcwidth actually matches standard library, in that zero-width and wide characters are silently allowed without error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants