-
Notifications
You must be signed in to change notification settings - Fork 87
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 supported versions and dependencies #509
Conversation
b94f6fe
to
b17a653
Compare
cd6a213
to
7b3d91e
Compare
as of 2.16 `wagtail.admin.views.generic.DeleteView` has been updated to match the Django 4.0 `DeleteView` implementation which uses `FormMixin` so the action happens in `form_valid` https://github.com/wagtail/wagtail/blob/main/docs/releases/2.16.md#wagtailadminviewsgenericdeleteview-follows-django-40-conventions
uses new lockfile version
067acab
to
7941c63
Compare
Codecov Report
@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 92.92% 92.37% -0.56%
==========================================
Files 36 36
Lines 3025 3055 +30
Branches 478 486 +8
==========================================
+ Hits 2811 2822 +11
- Misses 114 126 +12
- Partials 100 107 +7
Continue to review full report at Codecov.
|
7941c63
to
659b8fc
Compare
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.
Thanks so much for your work @zerolab ❤️
All looks good, but I do have one minor question about Django 3.0 and 3.1 missing from the tox dependencies. Perhaps you removed these by accident?
One other this I noticed is that we have some invalid dependency matrix combinations in Tox, we should probably clean that up (see comment for details). Not necessarily to be done in this PR but if you're up to it 🙂
polib>=1.1,<2.0 | ||
|
||
django2.2: Django~=2.2 | ||
django3.2: Django~=3.2 |
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.
Question: shouldn't Django 3.0 and 3.1 be listed here as they are still listed in envlist
?
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.
3.0 and 3.1 are no longer supported, so doing some tidyup there
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.
Gotcha! That makes sense.
wagtail2.13: wagtail>=2.13,<2.14 | ||
wagtail2.14: wagtail>=2.14,<2.15 | ||
wagtail2.15: wagtail>=2.15rc1,<2.16 | ||
wagtail2.11: wagtail~=2.11 |
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.
Praise: didn't know about the tilde equals ~=
in pip and looked it up, it has the same result as >=2.11,<2.12
but much shorter. Nicely done! 👍 😄
tox.ini
Outdated
@@ -2,7 +2,7 @@ | |||
skipsdist = True | |||
usedevelop = True | |||
|
|||
envlist = python{3.7,3.8,3.9}-django{2.2,3.0,3.1,3.2,main}-wagtail{2.11,2.12,2.13,2.14,2.15,main}-{sqlite,postgres} | |||
envlist = python{3.7,3.8,3.9}-django{2.2,3.0,3.1,3.2,main}-wagtail{2.11,2.12,2.13,2.14,2.15,2.16,main}-{sqlite,postgres} |
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.
Issue (non-blocking): this envlist is very large and contains some invalid combinations like python3.8-django2.2-wagtailmain-postgres
. These can't be tested because the Wagtail/Python/Django versions are not compatible.
Suggestion: we should clean this up (in a follow-up PR?) to remove invalid combinations. I've done something similar in the past for wagtail-live by splitting the envlist over multiple lines. See https://github.com/wagtail/wagtail-live/blob/main/tox.ini#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.
great suggestion. was thinking the same last night.
we're doing that in wagtailmedia - https://github.com/torchbox/wagtailmedia/blob/main/tox.ini#L5-L8
will get on it. may move the version definition in own include as I did in torchbox/wagtailmedia@0317d44 - should be cleaner than explicitly installing the testing requirements in tox
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.
Thanks! Happy to re-review if needed 🙂
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.
659b8fc
to
170f17a
Compare
170f17a
to
9f36f7e
Compare
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.
Thanks again @zerolab
Some small changes to be made to tox.ini and setup.py. Everything else looks fabulous!
tox.ini
Outdated
py{3.7,3.8,3.9}-django{2.2}-wagtail{2.11,2.12,2.13}-{sqlite,postgres} | ||
py{3.7,3.8,3.9}-django{3.2}-wagtail{2.14,2.15,2.16}-{sqlite,postgres} | ||
py{3.7,3.8,3.9}-django{4.0}-wagtail2.16-{sqlite,postgres} | ||
py{310}-dj{3.2,main}-wagtail{2.15,2.16,main}-{sqlite,postgres} |
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.
Adding the dots to the i, so to speak 😉
py{310}-dj{3.2,main}-wagtail{2.15,2.16,main}-{sqlite,postgres} | |
py{3.10}-dj{3.2,main}-wagtail{2.15,2.16,main}-{sqlite,postgres} |
tox.ini
Outdated
@@ -2,7 +2,11 @@ | |||
skipsdist = True | |||
usedevelop = True | |||
|
|||
envlist = python{3.7,3.8,3.9}-django{2.2,3.0,3.1,3.2,main}-wagtail{2.11,2.12,2.13,2.14,2.15,main}-{sqlite,postgres} | |||
envlist = | |||
py{3.7,3.8,3.9}-django{2.2}-wagtail{2.11,2.12,2.13}-{sqlite,postgres} |
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.
Suggestion: the basepython
section should be updated to match. Tox doesn't know what py3.7 means, since you've changed it from python3.7
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 should not do this type of changes while travelling on a bus :)
good spot. updated to use python
as previously, and dotted the 3.10 as well as added the definition in the basepython
section
@@ -41,9 +44,9 @@ | |||
"Framework :: Wagtail", |
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.
chore: did not note this in my previous review: but since we don't support Django 3.0 and 3.1 anymore, those classifiers should be removed from this section.
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.
✅
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.
TY ❤️
b7a63b3
to
5cc1748
Compare
This PR:
main
branch).nvmrc