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

Switch to PrettyTable #82

Closed
jayvdb opened this issue Nov 24, 2020 · 13 comments
Closed

Switch to PrettyTable #82

jayvdb opened this issue Nov 24, 2020 · 13 comments

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Nov 24, 2020

PrettyTable is now maintained by jazzband and a new release is out.

@raimon49
Copy link
Owner

Hmm, that's a breaking change to be considered in the next major release.

@johnthagen
Copy link
Contributor

I came here to suggest the same thing. Since PTable hasn't seen a release since 2015, it's basically abandoned. It is nice to see prettytable alive again: https://github.com/jazzband/prettytable

@raimon49
Copy link
Owner

I was just following the implementation of the prettytable package for another issue and noticed one thing.

In the current version of prettytable 2.5.0, the number of dependent packages has increased.

https://github.com/jazzband/prettytable/blob/2.5.0/setup.cfg

[options]
install_requires =
    wcwidth
    importlib-metadata;python_version < '3.8'

So, after switching from PTable to prettytable, we need to decide whether we should add more system packages?

ref) #119 #117 #89

@johnthagen
Copy link
Contributor

So, after switching from PTable to prettytable, we need to decide whether we should add more system packages?

This is a good point... Ideally there wouldn't need to be extra system packages, but I do think that anything that pip-licenses depends on should be excluded by default since it will be extremely likely that the end user isn't actually depending on them.

Perhaps the additional dependencies are a motivation to stay with PTable if it's not causing issues? How would you rate the current maintenance burden of PTable not being maintained?

Then there is the extra question of what happens when prettytable decides to add extra dependencies, some of which could be common?

@raimon49
Copy link
Owner

Perhaps the additional dependencies are a motivation to stay with PTable if it's not causing issues? How would you rate the current maintenance burden of PTable not being maintained?

Yes, in the case where pip-licenses outputs a license list, I would rate PTable as stable enough. However, we may have an unknown bug, or it may not work in future Python versions.

Then there is the extra question of what happens when prettytable decides to add extra dependencies, some of which could be common?

I read a bit about how the dependency was added to prettytable. It was to better handle Unicode, such as CJK. As I am also a Japanese speaker, I understand this addition. However, many open source licenses are written in English, which is a feature that pip-licenses do not need. prettytable is being developed on a community basis, and more features will be added in the future.

I have one idea and solution. That's vendoring original PTable code in pip-licenses and distributes it.

-from prettytable import PrettyTable
+from ._vendor.prettytable import PrettyTable

 SYSTEM_PACKAGES = (
     __pkgname__,
     'pip',
-    'PTable' if PTABLE else 'prettytable',
     'setuptools',
     'wheel',
 )

This idea will resolve the dependency of pip-licenses on external packages.

That's the problem with this idea:

@johnthagen
Copy link
Contributor

I have one idea and solution. That's vendoring original PTable code

That is a great idea I hadn't considered. It basically solves two issues because now you don't have to consider it a system package and have the ability to fix an issue if one arises. If there are no issues, then nothing needs to be changed, right?

I think this is a great path forward.

@johnthagen
Copy link
Contributor

johnthagen commented Dec 22, 2021

Another similar option would be to fork and release PTable2. The down side to this is that others might ask you to maintain and update it, which might be out of scope for what you want to do with your spare time.

@jayvdb
Copy link
Contributor Author

jayvdb commented Dec 22, 2021

distros have policies that require de-vendor packages, because they require separate copyright metadata for each component with different author/copyright/etc. And PTable cant be de-vendored because it clashes with PrettyTable.
In effect, vendoring PTable is going to mean pip-licenses becomes hard to maintain as a distro package, and probably will be dropped. Currently https://repology.org/project/python:pip-licenses/versions shows it is only me maintaining SUSE and someone else maintaining FreeBSD, but that list will grow if pip-licenses stays distro-friendly.

Note https://pypi.org/project/ptable2/ already exists, and it uses prettytable as its install directory, so the same clash problem exists. You could release as PTable3 and install into ptable3 to avoid the namespace clash, but I would have a hard time justifying creating a distro package for this new PTable3 which only has a single use: pip-licenses.

I would suggest vendoring the jazzband PrettyTable , and either removing the extra deps, slightly reducing the functionality, or also vendoring those. I would argue that pip-license needs to be multi-lingual as I (as creator of https://github.com/jayvdb/pypidb) have seen plenty of python package metadata needing wcwidth. But if you are going to vendor, as maintainer of the SUSE pip-licenses, I dont mind if I have to de-vendor PrettyTable - that is doable. There is no clash to worry about.

The tool https://pypi.org/project/vendoring/ should be suitable for recursive vendoring, if you want to go down that path. Its warning about not using the tool is effectively repeating what I've said above.

@raimon49
Copy link
Owner

@jayvdb Thanks for the information.

I understand that there is a problem with the installation directory. I usually use a various distro packages and have a lot of respect for the maintainers.

In reality, in some distro environments, the combination of pip-licenses and jazzband PrettyTable is broken and does not work as reported in #119. This is a problem we want to solve.

I would suggest vendoring the jazzband PrettyTable , and either removing the extra deps, slightly reducing the functionality, or also vendoring those. I would argue that pip-license needs to be multi-lingual as I (as creator of https://github.com/jayvdb/pypidb) have seen plenty of python package metadata needing wcwidth. But if you are going to vendor, as maintainer of the SUSE pip-licenses, I dont mind if I have to de-vendor PrettyTable - that is doable. There is no clash to worry about.

The tool https://pypi.org/project/vendoring/ should be suitable for recursive vendoring, if you want to go down that path. Its warning about not using the tool is effectively repeating what I've said above.

Thanks, I've touched up the vendoring package a bit too. This tool is optimized for pip development, which is a bit over-specified. For example, we do not currently maintain pyproject.toml either.

It is a difficult choice whether to vendoring wcwidth as well as a single package... Your advice has been greatly helpful.

@raimon49
Copy link
Owner

I currently don't have much time to spend on this project, but we need to resolve the issue of dependence on PTable.

I get the following message when I pip install .

Installing collected packages: PTable
  DEPRECATION: PTable is being installed using the legacy 'setup.py install' method, because it does not have a 'pyproject.toml' and the 'wheel' package is not installed. pip 23.1 will enforce this behaviour change. A possible replacement is to enable the '--use-pep517' option. Discussion can be found at https://github.com/pypa/pip/issues/8559
  Running setup.py install for PTable ... done

@johnthagen
Copy link
Contributor

Switching to prettytable is probably the best option. We have a test suite, which should at least detect if there are regressions.

Is that what you think is the best option?

@raimon49
Copy link
Owner

I agree the best option is to switch to jazzband/prettytable.

Much time is needed to realize the migration plan.

raimon49 added a commit that referenced this issue Oct 27, 2022
* Complete migration of table output library from PTable to prettytable
* Remove compatibility to work with either PTable or prettytable
* Fix failing tests with prettytable #120
@raimon49
Copy link
Owner

raimon49 commented Nov 6, 2022

This issue has been resolved in pip-licenses 4.x
https://pypi.org/project/pip-licenses/4.0.0/

@raimon49 raimon49 closed this as completed Nov 6, 2022
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

No branches or pull requests

3 participants