search

jazzband / prettytable

Display tabular data in a visually appealing ASCII table format
https://pypi.org/project/PrettyTable/
Other
1.32k stars 149 forks source link

Change how size of headers are calculated to fix ColorTable with title #299

Closed av-guy closed 2 months ago

av-guy commented 3 months ago

Fixes #290

The issue that the ColorTable is experiencing deals with its incorporation of ANSI escape character sequences. The ColorTable class works by incorporating ANSI escape character sequences both before and after the character it represents. That creates a discrepancy in how PrettyTable and ColorTable portray the same character.

I altered how _stringify_title computes its width. Instead of relying on self._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_header slices the bits value. The old code used hard values of 1 and -1, which doesn't account for the length of a single character when someone uses ColorTable.

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 both ColorTable and PrettyTable with these recent changes.

av-guy commented 3 months ago

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 main and pass with this PR, so we can avoid regressions in the future.

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?

hugovk commented 3 months ago

Looks good, except please move "Fixes #xxx" from the title to PR the description.

av-guy commented 3 months ago

Looks good, except please move "Fixes #xxx" from the title to PR the description.

I added Fixes #290 to my original comment. I don't contribute very often, so if I did this wrong, please let me know.

av-guy commented 3 months ago

Looks good, except please move "Fixes #xxx" from the title to PR the description.

I added a basic test for the color table. Please see commit df8f2e6 and provide feedback as necessary.