Skip to content

Conversation

mkmkme
Copy link
Collaborator

@mkmkme mkmkme commented Oct 7, 2025

Highly inspired by #96 by @ds-cbo (Thanks!)

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

Switch from setup.py to pyproject.toml

@mkmkme
Copy link
Collaborator Author

mkmkme commented Oct 7, 2025

@trim21 would you be able to review this one? Thanks!

Signed-off-by: Mikhail Koviazin <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (b59ee44) to head (dc3518a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #230      +/-   ##
==========================================
- Coverage   76.27%   76.26%   -0.01%     
==========================================
  Files         130      129       -1     
  Lines       33913    33911       -2     
==========================================
- Hits        25867    25863       -4     
- Misses       8046     8048       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -0,0 +1,96 @@
[build-system]
requires = ["setuptools>=61.0", "wheel"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wheel is not necessary anymore

]

[tool.setuptools.package-data]
valkey = ["py.typed"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also not necessary

@mkmkme
Copy link
Collaborator Author

mkmkme commented Oct 8, 2025

Thanks everyone for the reviews! I'll double-check tonight whether the changes in the comments are really needed and will either fix or merge this

[project.optional-dependencies]
libvalkey = ["libvalkey>=4.0.1"]
ocsp = ["cryptography>=36.0.1", "pyopenssl==23.2.1", "requests>=2.31.0"]
dev = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just an fyi

i think? people can now do pip install valkey[dev] and get from pypi
same thing goes for doc

some package managers (uv, poetry) have a way to prevent this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a great point.

If there's a way to do that with pyproject itself, I'm all for it.

Alternatively, we can also do what redis-py has done, that is switching to pyproject but keeping dev_requirements.txt and docs/requirements.txt. Honestly this sounds like the best approach.

@bogdanp05 WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/django-commons/django-valkey/blob/main/pyproject.toml#L60

this clause is documented in pyproject docs

but if i understand correctly it needs a lock file to work, so a package manager is needed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a chance my data is outdated and it can be used as is
https://packaging.python.org/en/latest/specifications/dependency-groups/

Copy link
Contributor

@trim21 trim21 Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use dependency-groups and replace pip with uv in our ci would help. replace pip install .[dev] with uv sync --group dev

Copy link
Contributor

@amirreza8002 amirreza8002 Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i just found out you can use pip to install dependency groups
tho it's a new feature (like 5 month old)
pip install --group test

good thing about pyproject is that it's a standard format, so you can replace pip with uv or poetry or vis versa and it just works

@amirreza8002
Copy link
Contributor

also you might consider a build tool that's not setuptools ;)

it lacks some features, I've had cases where it couldn't even publish to pypi cause it lacked the new standard

@mkmkme
Copy link
Collaborator Author

mkmkme commented Oct 8, 2025

also you might consider a build tool that's not setuptools ;)

it lacks some features, I've had cases where it couldn't even publish to pypi cause it lacked the new standard

Thanks a lot! I'll look into it. Do you have any suggestions for the alternative?

@trim21
Copy link
Contributor

trim21 commented Oct 8, 2025

also you might consider a build tool that's not setuptools ;)
it lacks some features, I've had cases where it couldn't even publish to pypi cause it lacked the new standard

Thanks a lot! I'll look into it. Do you have any suggestions for the alternative?

flit-core

@mkmkme
Copy link
Collaborator Author

mkmkme commented Oct 8, 2025

also you might consider a build tool that's not setuptools ;)
it lacks some features, I've had cases where it couldn't even publish to pypi cause it lacked the new standard

Thanks a lot! I'll look into it. Do you have any suggestions for the alternative?

Note to myself: redis-py uses hatchling, might worth looking into it as well.

@amirreza8002
Copy link
Contributor

amirreza8002 commented Oct 8, 2025

i use hatch everywhere

it does the job without noise

but I'm not an expert in this matter and don't have a strong suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants