search

KitwareMedical / dicom-anonymizer

Tool to anonymize DICOM files according to the DICOM standard
BSD 3-Clause "New" or "Revised" License
104 stars 47 forks source link

Define the dependencies in only one place using setup.py #81

Closed ue-sho closed 5 months ago

ue-sho commented 6 months ago

This PR (https://github.com/KitwareMedical/dicom-anonymizer/pull/66) introduced pipenv, but the documentation and GitHub Actions use venv, so developers are confused as to which to use. Therefore, I modified it to use pipenv. In addition, the name of the GitHub Actions was changed from tester.yml to ci.yml because I will introduce Ruff to check lint and format (https://github.com/KitwareMedical/dicom-anonymizer/issues/75). After this PR is merged, I intend to create a PR that will introduce Ruff.

pchoisel commented 6 months ago

Hi, thanks for your contribution !
To be fair, I'm not fond of pipenv. Because of it, we now have a third place where we have to define the dependencies: setup.py (for Python3.6 compat), pyproject.toml (for Python3.12 support) and Pipfile.
This is likely to create problems when we want to change the dependencies... Apart from being a bit easier to easier to use than venv, is there really a good reason to use this ? (I know I should have asked that question in #66...) @smjoshiatglobus Do you have an opinion on this ?

Apart from that, the rest of you contributions (renaming tester.yaml to ci.yaml and adding cache in the CI) are good for me.

smjoshiatglobus commented 6 months ago

Hi, thanks for your contribution ! To be fair, I'm not fond of pipenv. Because of it, we now have a third place where we have to define the dependencies: setup.py (for Python3.6 compat), pyproject.toml (for Python3.12 support) and Pipfile. This is likely to create problems when we want to change the dependencies... Apart from being a bit easier to easier to use than venv, is there really a good reason to use this ? (I know I should have asked that question in #66...) @smjoshiatglobus Do you have an opinion on this ?

Apart from that, the rest of you contributions (renaming tester.yaml to ci.yaml and adding cache in the CI) are good for me.

I agree that defining dependencies in three places is not a good idea. Duplicating dependencies is worse! To be fair to pipenv, the dependencies in pipenv are optional and meant for developers of this package while those in pyproject.toml are for others who only import the package. I switched to pyproject.toml after observing that pydicom had switched to it. When I created PR #66, the supported version of python was >= 3.7. That has moved to 3.6 now.

It makes sense to define the dependencies in only one place. Since setup.py will work for all versions of python, I support dropping both pyproject.toml and Pipfile and sticking to only setup.py.

ue-sho commented 6 months ago

Given that setup.py supports all versions of Python you are dealing with, I too think it would be wise to consolidate dependency management there. I am going to change the installation to using setup.py.

ue-sho commented 6 months ago

On second thought, I think it would be better to use pyproject.toml. The pyproject.toml file provides a more modern and simplified management of the build system instead of using setup.py.

Since setuptools supports pyproject.toml, I think it should also work with python 3.6. https://github.com/pypa/setuptools/pull/2970

So I will try to find a way to implement it using pyproject.toml.

ue-sho commented 5 months ago

@pchoisel If you merge this PR, I will immediately create a PR that will check the format and lint of the code by ruff.

pchoisel commented 5 months ago

Thank you !