-
Notifications
You must be signed in to change notification settings - Fork 14
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
Drop support for Python 3.8 #295
Conversation
9b7c8d7
to
2412ae2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #295 +/- ##
==========================================
+ Coverage 94.82% 95.50% +0.67%
==========================================
Files 24 24
Lines 1991 1979 -12
==========================================
+ Hits 1888 1890 +2
+ Misses 103 89 -14 ☔ View full report in Codecov by Sentry. |
@mrtmm Just checking if you want my review here. (I'm marked as reviewer but the PR is still a Draft. :) ) |
@fghaas yes I initially added you to review before I saw the tests failing. Now is ready to be reviewed (although still I need to look into the coverage). |
Honestly, I'm not sure exactly what codecov is complaining about, here. Having a low coverage rate on a diff that's mostly code removal doesn't really make sense to me. |
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.
This looks good to me in general. I have a couple of tiny nits, and then we also need an update to the README please.
8feb0eb
to
9699dc7
Compare
README.md
Outdated
| Nutmeg | `>=14.0, <15` | `>=7.0, <8.0` | `stable-7.0` | | ||
| Olive | `>=15.0, <16` | `>=7.5, <8.0` | `stable-7.0` | | ||
| Palm | `>=16.0, <17` | `>=7.5, <8.0` | `stable-7.0` | | ||
| Quince | `>=17.0, <18` | `>=7.9, <8.0` | `stable-7.0` | |
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.
stable-7.0
doesn't work for a branch where the latest minor release is 7.13. :) You probably want to call the branch stable-7
instead.
Okay. Final thought: do we want to add python_requires='>=3.11', ... to (Reference: |
Now that we're enforcing a minimum of Python 3.11, we'll also need: basepython = python3.11 in Also, can you please check that that doesn't break anything when running |
I currently run python3.12 tests locally only and rely the rest to be run in here; adding |
* Drop support for Python 3.8 and Xblock <2. * Remove Python 3.8 and pip 22.0.4 from the test matrix. * Use generative section names in tox.ini By prefixing *all* our testenvs with the Python version factor ("py311", "py312", etc.), we can ensure that all of our testenvs can be invoked with either Python version being present on the system. tox will then skip those environments that don't match the Python present on the system, and — provided the remaining environments succeed — return with a zero exit code (indicating success). However, if *all* environments are skipped, because there is no matching Python version present on the system at all, it returns with a nonzero exit code (indicating failure). Reference: https://tox.wiki/en/latest/config.html#generative-section-names
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 good to me now; please merge at your convenience. Thanks!
No description provided.