pytest-dev / pytest-fixture-tools

Pytest fixture tools
MIT License
38 stars 6 forks source link

Fix #8 - Remove migrate setup.py to pyproject.toml #9

Closed ogajduse closed 1 month ago

ogajduse commented 1 month ago

Invocation of python setup.py command is deprecated. Therefore the test command was removed.

Everything else except the detox optional (test) dependency was migrated over to pyproject.toml. detox is not used anywhere in the project anymore.


This change is Reviewable

ogajduse commented 1 month ago

@webknjaz Thank you for all your comments! TIL something new. I have addressed all your comments. I removed the extra fields not present in setup.py. I will add them in a separate PR.

After building both and comparing them, the only real difference that I spotted was in classifiers. And also the changelog is not included in the metadata.

Full diff here (expires in 1 day): https://paste.centos.org/view/270b42dc

--- ./pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/METADATA  2024-08-19 13:56:48.420288462 +0200
+++ /tmp/pft-pyproject-dist/dist/pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/METADATA       2024-08-19 13:55:41.449135453 +0200
@@ -2,9 +2,7 @@
 Name: pytest-fixture-tools
 Version: 1.2.0
 Summary: Plugin for pytest which provides tools for fixtures
-Author: Paylogic International
-Author-email: developers@paylogic.com
-License: MIT license
+Author-email: Paylogic International <developers@paylogic.com>
 Classifier: Development Status :: 6 - Mature
 Classifier: Intended Audience :: Developers
 Classifier: License :: OSI Approved :: MIT License
@@ -21,10 +19,13 @@
 Classifier: Programming Language :: Python :: 3.10
 Classifier: Programming Language :: Python :: 3.11
 Classifier: Programming Language :: Python :: 3.12
+Description-Content-Type: text/x-rst
 License-File: LICENSE.txt
 Requires-Dist: pytest
 Requires-Dist: pydot
 Requires-Dist: py
+Provides-Extra: test
+Requires-Dist: detox ; extra == 'test'

Is that an acceptable diff?

webknjaz commented 1 month ago

I see the license field is missing. So you need to recover it.

Also, the changelog part of the long description needs to be added as it was originally. Do this via making the readme field dynamic, it then lets you list several files in the config. It's documented somewhere in the setuptools docs.

webknjaz commented 1 month ago

Regarding the extra, using them as dev requirements is abusing their true purpose. They're public API for additional runtime deps. This lets the end-users pip install prj[test], however this is only useful to the contributors. I'm hoping the dependency groups PEP will get finished soon to close this gap. Personally, I'm in the camp of using requirement files for dev/test envs. So just delete that extra declaration. It's not used anywhere and there's no need to declare detox as an additional runtime dep.

webknjaz commented 1 month ago

By the way, the tox and Travis CI configs call py.test directly. It needs to be changed to just pytest. But I don't see anything calling detox so I'd consider it a part of the same historic artifact as setup.py test that needs to be eliminated.

P.S. I think I had a good pyproject.toml example under cherrypy/cheroot if you need another example (except for some extras antipatterns due to historic reasons).

ogajduse commented 1 month ago

Well, I guess that I can not make the diff smaller than the following.

--- pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/METADATA    2024-08-19 16:04:09.786743131 +0200
+++ /tmp/pft-pyproject-dist/dist/pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/METADATA   2024-08-19 16:31:49.972434313 +0200
@@ -2,8 +2,7 @@
 Name: pytest-fixture-tools
 Version: 1.2.0
 Summary: Plugin for pytest which provides tools for fixtures
-Author: Paylogic International
-Author-email: developers@paylogic.com
+Author-email: Paylogic International <developers@paylogic.com>
 License: MIT license
 Classifier: Development Status :: 6 - Mature
 Classifier: Intended Audience :: Developers
@@ -21,6 +20,7 @@
 Classifier: Programming Language :: Python :: 3.10
 Classifier: Programming Language :: Python :: 3.11
 Classifier: Programming Language :: Python :: 3.12
+Description-Content-Type: text/x-rst
 License-File: LICENSE.txt
 Requires-Dist: pytest
 Requires-Dist: pydot
diff -r -u --color pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/RECORD /tmp/pft-pyproject-dist/dist/pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/RECORD
--- pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/RECORD  2024-08-19 16:04:09.787743132 +0200
+++ /tmp/pft-pyproject-dist/dist/pytest_fixture_tools-1.2.0/pytest_fixture_tools-1.2.0.dist-info/RECORD 2024-08-19 16:31:49.973434315 +0200
@@ -1,7 +1,7 @@
 pytest_fixture_tools/__init__.py,sha256=47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU,0
 pytest_fixture_tools/plugin.py,sha256=4vrq9Hu3z2R1_TrLhAgn6mUJnZe0TIJxaz0zvUataNI,5686
 pytest_fixture_tools-1.2.0.dist-info/LICENSE.txt,sha256=SeotkfqWJxQYQcjx0r3VW753u1SMaALMn1Yghut7olk,1060
-pytest_fixture_tools-1.2.0.dist-info/METADATA,sha256=uW-t6d81yIjTTgu5rKFvlYxPxWxoTmzWzb5zSUQ-Bdg,5931
+pytest_fixture_tools-1.2.0.dist-info/METADATA,sha256=bLq02FXC-9WRytJSbLG40qN0laiQyAj_9URB3v0Ry1Y,5962
 pytest_fixture_tools-1.2.0.dist-info/WHEEL,sha256=HiCZjzuy6Dw0hdX5R3LCFPDmFS4BWl8H-8W39XfmgX4,91
 pytest_fixture_tools-1.2.0.dist-info/entry_points.txt,sha256=rZtYsl_4cbUWvbl-V4qJFrWcflfkISvSPSSvfYlfVX4,62
 pytest_fixture_tools-1.2.0.dist-info/top_level.txt,sha256=iNoT7ovfTat8HB3nqhht-cP2pGf3s3YtSZ7Nh6NpVP8,21

The change around authors is expected since

If both email and name are provided, the value goes in [Author-email](https://packaging.python.org/en/latest/specifications/core-metadata/#core-metadata-author-email) or [Maintainer-email](https://packaging.python.org/en/latest/specifications/core-metadata/#core-metadata-maintainer-email) as appropriate, with the format {name} <{email}>.

Also, the optional dependency on detox was removed as you suggested.

ogajduse commented 1 month ago

I will create a separate PR to switch from Travis CI to GitHub Actions.

ogajduse commented 1 month ago

It exactly matches. Thank you for your review, @webknjaz!

webknjaz commented 1 month ago

It exactly matches.

Good. I was worried about that empty-line in between those files. The setup.cfg feature with long_description = files: README.rst, CHANGELOG.rst didn't use to inject that empty line between files (IIRC I implemented the file list in setuptools back in the day).