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

Pad centered colored text consistently #60

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mgedmin
Copy link

@mgedmin mgedmin commented Mar 29, 2018

Fixes #55.

@anarcat
Copy link

anarcat commented Mar 29, 2018

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:

    def test_odd_width_height_pad_space(string, align, expected):
        """Test odd number width, height, padding, and dots for whitespaces.
    
        :param str string: String to test.
        :param str align: Alignment in any dimension but one at a time.
        :param list expected: Expected output string.
        """
        actual = align_and_pad_cell(string, (align,), (5, 3), (1, 1, 1, 1), '.')
>       assert actual == expected
E       AssertionError: assert ['.......', '....', '.......'] == ['.......', '.....', '.......']
E         At index 1 diff: '.test..' != '..test.'
E         Use -v to get the full diff
tests/test_width_and_alignment/test_align_and_pad_cell.py:205: AssertionError

@mgedmin
Copy link
Author

mgedmin commented Mar 29, 2018

Oops! I thought I ran tox before pushing, but apparently I forgot.

@mgedmin
Copy link
Author

mgedmin commented Mar 29, 2018

How does Python's str.center() decide where to put excessive padding? It seems inconsistent:

>>> 'test'.center(7, '.')  # extra padding on the left
'..test.'
>>> '00:00'.center(8, '.')  # extra padding on the right!
'.00:00..'

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.
@mgedmin
Copy link
Author

mgedmin commented Mar 29, 2018

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-io
Copy link

codecov-io commented Mar 29, 2018

Codecov Report

Merging #60 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
terminaltables/width_and_alignment.py 100% <100%> (ø) ⬆️
tests/test_all_tables_e2e/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad8f46e...c00687f. Read the comment docs.

Copy link

@anarcat anarcat left a 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.

@faudebert
Copy link

I can confirm I have better results regarding centering with this PR merged. Would appreciate a release including it.

@anarcat
Copy link

anarcat commented Jul 24, 2019

ping - any reason why this isn't merged yet? see also #70...

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.

center-formatting broken with certain escape sequences
4 participants