-
Notifications
You must be signed in to change notification settings - Fork 73
Pad centered colored text consistently #60
base: master
Are you sure you want to change the base?
Conversation
hmmm... this makes other tests fail in the suite... so yeah, maybe a policy decision to make here, but at least the tests should be fixed before this is merged. :) travis says this:
|
Oops! I thought I ran |
How does Python's str.center() decide where to put excessive padding? It seems inconsistent:
I wanted to be consistent with str.center() and thought it would always put the extra padding on the right. I was wrong. |
CPython's str.center() does unicode_center_impl(PyObject *self, Py_ssize_t width, Py_UCS4 fillchar) { Py_ssize_t marg, left; ... marg = width - PyUnicode_GET_LENGTH(self); left = marg / 2 + (marg & width & 1); return pad(self, left, marg - left, fillchar); } Here we have inner_dimensions[0] instead of width and extra_padding instead of marg.
CPython does this: marg = width - PyUnicode_GET_LENGTH(self);
left = marg / 2 + (marg & width & 1);
return pad(self, left, marg - left, fillchar); The extra padding goes to the left when both margin and width are odd. |
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
- Coverage 99.57% 99.56% -0.01%
==========================================
Files 8 7 -1
Lines 466 462 -4
Branches 76 76
==========================================
- Hits 464 460 -4
Misses 1 1
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as this works in Undertime and actually fixes #55, i'm all for this. i didn't look at the math in details, but it looks sound.
I can confirm I have better results regarding centering with this PR merged. Would appreciate a release including it. |
ping - any reason why this isn't merged yet? see also #70... |
Fixes #55.