-
-
Notifications
You must be signed in to change notification settings - Fork 144
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
Update to Python 3.11/pyproject.toml/current django versions #180
Conversation
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 the contribution ! Could we separate black upgrade / pre-commit upgrade / setup.py upgrade from the slugify changes, please ? I like them but let's separate things.
@Pierre-Sassoulas Sure. The problem is I couldn't run/test it without those at all. I can make a PR just with those changes first and then make this one depend on that other one. |
@Pierre-Sassoulas done |
|
||
setup() | ||
setup(include_package_data=True, packages=find_packages()) |
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 think this can also go in pyproject.toml, (or it become useless), no ?
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.
yes, however, I saw this listed as still "beta" in the documentation as of today, which is why I moved it back into setup.py. I did the same for my own projects 1-2 months ago. It's possible that it will just work and that we can ignore the beta note in the documentation. https://setuptools.pypa.io/en/latest/userguide/package_discovery.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.
Fair enough, thank you for the explanation :)
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.
Looks pretty good, thank you for the pre-commit upgrade too. I'm going to merge as soon as the pipeline is green. We can merge a small commits (for example black upgrade and new formatting) so the pipeline is run without my approval and you can tests easier (or you can use a PR in your own fork).
I think the github workflow needs to be modified too, we still have python 3.7 and no python 3.11. |
@Pierre-Sassoulas Oh yeah. You know there is actually also a github action plugin so that you only need to specify it in one place: https://github.com/ymyzk/tox-gh-actions |
@Pierre-Sassoulas I noticed you turned off github actions from running from outside contributors. I understand it is annoying for you to have to click "approve" all the time, but as far as I can tell, there is no option in github that allows me to run the github actions in my forked repo instead. |
I think it's possible if you open a pull request from a branch in your repo to your own main branch. |
@Pierre-Sassoulas I tried doing that. It didn't work: johanneswilm#1 . I tried looking through the settings and googling for solutions, but found nothing. Is there any particular reason for why you set it to requiring an approval for every single run? |
It's the default value imposed by github to prevent bitcoin mining :) |
Thank you for the multiplee upgrades, amazing ! |
Some of this is just minor changes needed to make it work with Python 3.11.2/pip 23.0.1.
Edit: Substantial changes have been moved to #181.