-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Add support for Python 3.8-3.10, drop EOL 2.7-3.5 #42
Conversation
cc @tomchristie as one of the major users; it looks like Starlette only supports 3.6+ as well, so this seems reasonable; what do you think? |
This would be cool. :) |
Starlette has now dropped support for EOL Python 3.6 as well: |
Looks okay to me. I'd typically nudge folks towards smaller sets of pull requests, but that'd probably be just adding unnecessary work at this point. Project probably needs a new dedicated maintainer tho. (I'd rather not be a primary maintainer on this project at this point - too many spinning plates) |
Can I help here? |
Well... possibly, tho you do already manage several different projects. |
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.
@hugovk Can you please enable the pipeline on pull requests on this PR?
I want to try to address the current issues. |
Unfortunately not, only people with write access can do that. Fortunately, I had enabled it for my fork and you can see the passing builds here: https://github.com/hugovk/python-multipart/actions |
@@ -8,7 +8,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python-version: [3.6, 3.7, 3.8, 3.9] |
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.
On the top of this file there's a "on: [push]".
Can you replace by this: https://github.com/encode/starlette/blob/bc61505faef15b673bf4bf65db4927daa354d6b8/.github/workflows/test-suite.yml#L4 ?
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'm personally strongly against adding this, it would have prevented me testing the CI on my fork and is an extra impediment to contributing.
https://github.com/hugovk/python-multipart/actions
Unless I edited the yml in my fork, and then remembered to revert it before creating a PR.
Or I can submit a PR on my branch first to check it passes -> extra impediment.
Or I can develop on my master
-> not a best practice, we should use feature branches, and I only have a single master
-> extra impediment.
The fact it's tested on upstream and on the fork isn't such a big deal: the testing is split between two different accounts and therefore doesn't use up any extra quota/time from any single account.
CIs allow us to test on a much larger matrix of Python versions and operating systems, much more than is possible locally and sometimes faster too. I sometimes avoid contributing to projects with no/broken CI.
I believe the use of CIs are of huge benefit for software quality and development, especially when so easy nowadays; Travis CI really revolutionised this, remember how hard testing was before. We should rather encourage people to test on their own forks before creating PRs.
Fail fast: shorten the time to finding a failure, enable quicker loops of incremental development.
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.
You're not against this. You're against GitHub blocking first time contributors for running their pipeline.
There's a setting to allow old GitHub users (3 months old ?) to not be blocked.
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.
You're not against this. You're against GitHub blocking first time contributors for running their pipeline.
I don't mind that bit too much :)
Consider this workflow:
I come along and fork the repo and enable the CI on my fork.
I create a feature branch add-3.10
.
I push my feature branch to my fork and check the CI.
I want the CI to test my changes on all supported Python versions (and possibly on several operating systems).
If it fails, I'll iterate: fix, push, repeat.
(This actually happened, see the first few failures as I iterated: https://github.com/hugovk/python-multipart/actions)
Once it passes, I can create a PR here, knowing that when the CI is enabled for the PR (we're 10 months and counting here), I'm confident it will pass and there's no surprises because I didn't test Python 3.7 or so on.
However, if we have these branch restrictions:
on:
push:
branches: ["master"]
pull_request:
branches: ["master"]
The CI won't build for me.
My options:
- Don't test on the CI, hope for the best when the PR runs here
- Edit the file and remove the restrictions, then before submitting the PR, remember to re-add the restrictions
- Develop in my
master
branch - not recommended, you can only work on one thing
All this applies even if I'm a second-time contributor.
Hi @tomchristie @andrew-d Can we proceed here? Also, I have bandwidth to maintain this project. 👀 |
Thanks for the merge! Please see #51 for the 2022 edition :) |
Thanks @hugovk 🙏 |
Add support for Python 3.8-3.10
Drop EOL 2.7-3.5
They're also little used. Here's the pip installs for python-multipart from PyPI for October 2021:
Source:
pip install -U pypistats && pypistats python_minor python-multipart --last-month
This means we can upgrade Python syntax and simplify imports, and remove six.