Change how size of headers are calculated to fix ColorTable with title #299
Conversation
There was a problem hiding this comment.
Thanks for the PR!
Please could you:
-
Update the title to be more descriptive? It will be used in the release notes. See https://github.com/jazzband/prettytable/releases to get an idea.
-
Add "Fixes #xxx" with the issue number to the PR description, so the issue will be closed when this is merged.
-
Add tests. It's good none of the existing tests fail. It will be even better to add tests which fail with current
mainand pass with this PR, so we can avoid regressions in the future.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
for more information, see https://pre-commit.ci
av-guy
changed the title
av-guy
changed the title
I will try to get the tests done today. I did change the PR description. Does the new description satisfy the first two requirements in your list? |
|
Looks good, except please move "Fixes #xxx" from the title to PR the description. |
av-guy
changed the title
I added Fixes #290 to my original comment. I don't contribute very often, so if I did this wrong, please let me know. |
for more information, see https://pre-commit.ci
I added a basic test for the color table. Please see commit df8f2e6 and provide feedback as necessary. |
hugovk
changed the title
Fixes #290
The issue that the
ColorTableis experiencing deals with its incorporation of ANSI escape character sequences. TheColorTableclass works by incorporating ANSI escape character sequences both before and after the character it represents. That creates a discrepancy in howPrettyTableandColorTableportray the same character.I altered how
_stringify_titlecomputes its width. Instead of relying onself._hrule, it now uses the padding widths and the widths of the columns themselves. From my testing, this seems to work well. I also modified the way_stringify_headerslices thebitsvalue. The old code used hard values of1and-1, which doesn't account for the length of a single character when someone usesColorTable.If you see anything that stands out, or you have input on a different solution, please let me know. I am open to the discussion. Also, I didn't see any tests related to the table rendering for
ColorTable, so I didn't add any. All of the original tests are passing for bothColorTableandPrettyTablewith these recent changes.