-
Notifications
You must be signed in to change notification settings - Fork 98
chore: modernize Python tooling #237
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
base: master
Are you sure you want to change the base?
Conversation
@No767 thanks for opening this PR. CI is not passing because python 3.7 is not supported on ubuntu latest, so I'd suggest removing "3.7" and "3.8" from the github workflow matrix as they are both EOL. 3.9 onwards seem to be supported. About git pre-commit hooks, I think they're great, but they won't replace a CI check. In my mind, the pre-commit hook is a way for devs to save time because you don't need to wait for feedback from CI, but CI still needs to validate ("never trust the client" sort of thing). |
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.
Thank you for getting this started! I think there's a general comment about not tightening version constraints too much, or various users might not be able to use this new version (in the past some users have mentioned they were still on older versions of python for instance, for lots of different reasons). In particular, the production dependencies should probably remain as open as possible. I think the dev/test ones are less problematic, but still.
authors = [ | ||
{ name = "Laurent Savaete", email = "[email protected]"}, | ||
{ name = "Noelle Wang", email = "[email protected]" } | ||
] |
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.
It would be nice to add all the people who actually contributed, what do you think? Or just have something like "slowapi contributors" with a link to the github repo?
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.
depending on if we have a list, then sure we can go for it
"hiro>=1.1.1,<2" | ||
] | ||
|
||
[tool.pyright] |
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 use pyright
when writing code (I'm not a VScode user), but I'm a bit hesitant to use it in CI, because it brings in the entire node stack with it, so more risks of breakage every now and then.
This does not prevent keeping the config here for people using it though.
As a side note, I just discovered https://github.com/astral-sh/ty which looks promising!
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.
Currently, the version of pyright
that is being used directly uses nodejs-wheel-binaries
, which does not require any installation of Nodejs. I've used this version with nodejs-wheel-binaries
within my own workflows, and have not needed to install Nodejs or anything
As there is a lot of outdated components, it would be best to just to replace them with modern tools such as uv and ruff. In addition, I've added Lefthook so now we have linting and formatting pre-commit, so this would ensure that our code is linted and formatted. Alternatively, another way of doing this is to have a GH actions workflow that checks the code, and a rule ensures that the workflow must pass in order to merge