Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Apr 28, 2024

Conversation

av-guy
Copy link
Contributor

@av-guy av-guy commented Apr 19, 2024

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.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

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.

src/prettytable/prettytable.py Outdated Show resolved Hide resolved
av-guy and others added 2 commits April 19, 2024 12:02
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@av-guy av-guy changed the title Bugfix colortable 290 Change how length of headers are calculated - Fixes #290 Apr 20, 2024
@av-guy av-guy changed the title Change how length of headers are calculated - Fixes #290 Change how size of headers are calculated - Fixes #290 Apr 20, 2024
@av-guy
Copy link
Contributor Author

av-guy commented Apr 20, 2024

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

hugovk commented Apr 20, 2024

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

@av-guy av-guy changed the title Change how size of headers are calculated - Fixes #290 Change how size of headers are calculated Apr 20, 2024
@av-guy
Copy link
Contributor Author

av-guy commented Apr 20, 2024

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
Copy link
Contributor Author

av-guy commented Apr 20, 2024

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.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thank you!

@hugovk hugovk changed the title Change how size of headers are calculated Change how size of headers are calculated to fix ColorTable with title Apr 28, 2024
@hugovk hugovk merged commit 0edf8e9 into jazzband:main Apr 28, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Fixed For any bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colortable with title leads to broken layout
2 participants