Skip to content

Conversation

disberd
Copy link
Contributor

@disberd disberd commented Sep 25, 2025

This PR update the code (and Project compat) to support version 3 of PrettyTables

Since version 3 brings breaking changes in the API of pretty tables, I decided to go with the branching solution based on the PrettyTables version rather than dropping compat to earlier versions.

I tested locally with v2 and tests still pass, don't know if one of the maintainers would want to add a CI test also testing with PrettyTables v2.

I think this update is needed for use of LocalCoverage in CI as this usually involves adding LocalCoverage to the global environment and then using it directly in a process where the current environment is the package under test.

In this scenario, despite LocalCoverage having a compat to v2, the v3 of PrettyTables is being loaded if that is a dependency of the package under test, leading to error at runtime when trying to generate the coverage.

@tpapp
Copy link
Collaborator

tpapp commented Sep 25, 2025

Thanks for the PR. As far as I am concerned we can just drop the previous version of PrettyTables and require the latest one, and not branch in the code.

@disberd
Copy link
Contributor Author

disberd commented Sep 25, 2025

Thanks for the fast reply @tpapp. I think the branching might still be useful for the reverse problem of what I was facing in CI.

If someones loads the new version of LocalCoverage only compatible with v3 of PrettyTables in the global env, but then has the test env which still depends on v2 of PrettyTables (migration is not always straighforward and I understand a bunch of packages are still locking to v2 of PrettyTables), the LocalCoverage code will still throw an error as it will try to use TextHighlighter which does not exist in the v2 of PrettyTables (loaded because the package under test in its environment does not allow v3 of PrettyTables).

This is still a easier problem to solve than currently (as it's sufficient to pin the LocalCoverage version in the global env when adding it prior to testing in CI), so if you still prefers to avoid branching I'll just update the PR accordingly

@tpapp
Copy link
Collaborator

tpapp commented Sep 25, 2025

This is a valid argument for the branches. So let's keep them; I will review in detail soon.

@disberd
Copy link
Contributor Author

disberd commented Sep 25, 2025

Thanks @tpapp, meanwhile I am also linking two PRs to PrettyTables with some docs inconsistencies I found while working on this PR:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants