jrief/django-admin-sortable2

Don't install testapp and its child modules

PeterJCLaw opened this issue · 8 comments

Thanks for providing this package, it's really useful.

It looks like the testapp used by this package is being installed alongside the intended code, even in the latest versions despite the exclusion of testapp from the packages searched by find_packages in setup.py. I think this is because there are sub-modules which find_packages is happily still including.

From some quick local testing I think the solution is to change from 'testapp' to 'testapp*'.

jrief commented

Thanks for reporting.

I removed folder testapp in version 2.1.9 but I was unable to find a folder named "submodules". Where did you find it?

Ah -- I meant the modules within testapp. Specifically there's a migrations folder with a 0001_initial.py and a __init__.py.

$ pip uninstall django-admin-sortable2
Found existing installation: django-admin-sortable2 2.1.9
Uninstalling django-admin-sortable2-2.1.9:
  Would remove:
    .../venv/lib/python3.11/site-packages/adminsortable2/*
    .../venv/lib/python3.11/site-packages/django_admin_sortable2-2.1.9.dist-info/*
    .../venv/lib/python3.11/site-packages/testapp/migrations/*
Proceed (Y/n)?
jrief commented

Everything below testapp is gone in version 2.1.9.

Please close if everything fits.

Hrm. My testing (as above) suggests that some files remain.

Here's the steps I'm using, on Ubuntu with Python 3.11:

python3.11 -m venv venv
. venv/bin/activate
pip install django-admin-sortable2
pip uninstall django-admin-sortable2

The last of these prints out the files which pip wants to remove -- which includes testapp/migrations/*.

Alternatively one can download the built wheel from PyPI and observe that within the archive there is a testapp directory in the root.

I can see that in 2.1.9 the source distribution doesn't contain the testapp folder. (I would encourage that it does though -- it's typically a good idea to include test code in the source distribution, even if those files are then not installed).

How does this compare to what you're doing to test this?

jrief commented

I can see that in 2.1.9 the source distribution doesn't contain the testapp folder. (I would encourage that it does though -- it's typically a good idea to include test code in the source distribution, even if those files are then not installed).

This in my opinion is an antipattern. Having testing code in the repository attached to a CI pipeline makes perfectly sense, but installable packages should (imo) be as small as possible. Therefore I prefer not adding any testing code to a package published on PyPI .

Would you be open to a PR which fixes this for built wheels?

jrief commented

As I wrote 5 days ago, subfolder testapp is not part of version 2.1.9 anymore, so I don't see any benefit for this PR.

As I wrote 5 days ago, subfolder testapp is not part of version 2.1.9 anymore, so I don't see any benefit for this PR.

You are correct that the published source distribution (.tar.gz) no longer contain those files. However I believe the published wheel still contains and installs files from testapp/migrations, as I demonstrated above (#359 (comment)). Please test the wheel published on PyPI (or one locally built from the repo) for yourself.