-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Define the dependencies in only one place using setup.py #81
Define the dependencies in only one place using setup.py #81
Conversation
7dd5bde
to
1b178e9
Compare
c5c5264
to
e6d963a
Compare
Hi, thanks for your contribution ! 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 It makes sense to define the dependencies in only one place. Since |
Given that |
On second thought, I think it would be better to use Since So I will try to find a way to implement it using |
e6d963a
to
504853a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I will try to find a way to implement it using pyproject.toml.
When Python version is 3.6, the installation of editable mode (pip install -e .[dev]
) fails due to low version of setuptools
.
Therefore, I decided to define it in setup.py
.
@pchoisel @smjoshiatglobus
Please review when you have time.
.github/workflows/ci.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed that CI works fine using act.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment on a dead link in setup.py
. Otherwise LGTM.
setup.py
Outdated
@@ -27,4 +44,13 @@ | |||
# For an analysis of "install_requires" vs pip's requirements files see: | |||
# https://packaging.python.org/en/latest/requirements.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# https://packaging.python.org/en/latest/requirements.html | |
# https://packaging.python.org/en/latest/guides/distributing-packages-using-setuptools/ |
The link on line 45 is now dead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it.
504853a
to
0ad9ea4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@pchoisel |
Thank you ! |
This PR (#66) introduced
pipenv
, but the documentation and GitHub Actions usevenv
, 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
toci.yml
because I will introduce Ruff to check lint and format (#75).After this PR is merged, I intend to create a PR that will introduce Ruff.