valkey-io / valkey-py

Valkey Python client based on a fork of redis-py
MIT License
69 stars 9 forks source link

update setup.py to pyproject.toml #96

Open ds-cbo opened 2 months ago

ds-cbo commented 2 months ago

Pull Request check-list

Description of change

I upgraded the deprecated setup.py to the modern pyproject.toml format, enabling modern installer tools.

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.16%. Comparing base (c8c19c2) to head (fcb7b67). Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
setup.py 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #96 +/- ## ========================================== + Coverage 75.12% 75.16% +0.04% ========================================== Files 132 132 Lines 34398 34480 +82 ========================================== + Hits 25840 25918 +78 - Misses 8558 8562 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mkmkme commented 2 months ago

Thanks for your contribution.

One question though: why do you think setup.py is deprecated? This web page explicitly says it is not.

ds-cbo commented 2 months ago

Interesting, those articles contradict what I see other big projects doing, even if they keep using setuptools as build system:

Internally we also moved to pyproject by removing setup.py because we believed that was the new way to go, I didn't even know that you could keep both files in tandem.

Would you like me to bring back a barebones from setuptools import setup ; setup() setup.py?

aiven-sal commented 2 months ago

Did you encounter any specific issues with setup.py or is this intended more as a cosmetic change?

aiven-sal commented 2 months ago

BTW yes, please don't remove setup.py completely.

As a side note, the sign-off thingy should contain a real name and a real email address

ds-cbo commented 2 months ago

@aiven-sal I encountered issues with our build system which requires pyproject.toml be available in libraries we use. Of course we could also put some patches in our repo only, but I imagined it might be useful to more people to contribute upstream too.

I've added the setup.py in the last commit.

What do you mean with "real name" and "real email address"? These are the two linked to this GitHub account, I imagined those would be more useful than some generic untraceable values.

aiven-sal commented 2 months ago

What do you mean with "real name" and "real email address"? These are the two linked to this GitHub account, I imagined those would be more useful than some generic untraceable values.

Maybe this can clarify what I mean: https://github.com/cncf/foundation/blob/main/dco-guidelines.md#dco-and-real-names
The "real name" doesn't necessarily have to be the name on your ID, but it shouldn't be an "anonymous id" and the email needs to be an actual email address that can be used to contact you directly.

aiven-sal commented 2 months ago

To put it in a different way. Imagine we just met at a conference, what name would you use to introduce yourself? That should be the name you use in signoff

ds-cbo commented 2 months ago

I guess that would be it then?

mkmkme commented 2 months ago

The CI has failed. Could you please fix it accordingly?

It seems that:

  1. Words pyproject and toml should be added to the dictionary
  2. The CI is using python setup.py directly which is no longer correct
ds-cbo commented 2 months ago

@mkmkme I've just updated the dictionary, but as far as I know I already replaced all instances of python setup.py from CI in a previous commit. Am I still missing something (and if so: where?), or were you perhaps looking at an older CI run?

Do you also expect me to pass the (seemingly unrelated and/or outdated?) pip-audit checks?

mkmkme commented 2 months ago

@ds-cbo There's still a bunch of leftovers that you can find by https://github.com/search?q=repo%3Avalkey-io%2Fvalkey-py%20setup.py&type=code

IMO all of them should be fixed. The one that is causing the CI to fail is under .github/workflows. The ones in INSTALL and tasks.py are user-facing, so I'd change them, too.

Thanks! :)

mkmkme commented 2 months ago

Do you also expect me to pass the (seemingly unrelated and/or outdated?) pip-audit checks?

We certainly can't merge the PR with CI failing. If you don't want to handle this, I can do it myself.

mkmkme commented 2 months ago

I am also quite concerned that the direct calls to python setup.py from the CI are failing despite the fact we do have setup.py now

ds-cbo commented 2 months ago

@ds-cbo There's still a bunch of leftovers that you can find by https://github.com/search?q=repo%3Avalkey-io%2Fvalkey-py%20setup.py&type=code

IMO all of them should be fixed. The one that is causing the CI to fail is under .github/workflows. The ones in INSTALL and tasks.py are user-facing, so I'd change them, too.

Thanks! :)

As far as I know, I've already modified those in earlier commits. See for example https://github.com/ds-cbo/valkey-py/blob/pyproject.toml/.github/workflows/install_and_test.sh#L23

We certainly can't merge the PR with CI failing. If you don't want to handle this, I can do it myself.

Understandable. I wouldn't mind fixing it, but I don't really know your preferred course of action here. It's failing on Babel 2.8.0 even though 2.16.0 is already available and nothing seems to prevent it from being installed. Maybe sphinx<7.0 in our dependency list is blocking it? The last sphinx<7.0 would be 6.2.1 from april 2023. I have no clue what the impact of that major upgrade would be on this project though, so I don't want to mindlessly remove the upper pin.

I'm guessing this change happened because now we're running pip-audit including the docs dependencies, where previously we'd only run it on dev? I'm a bit unsure

I am also quite concerned that the direct calls to python setup.py from the CI are failing despite the fact we do have setup.py now

I still think you're looking at older CI reports, the reason the newest packaging runs were failing was because build wasn't available, and I've just pushed a commit to fix that.