Skip to content
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

Support/wagtail-50 #457

Merged
merged 2 commits into from
Jun 9, 2023
Merged

Conversation

katdom13
Copy link

@katdom13 katdom13 commented Jun 8, 2023

Wagtail 5.0 release notes: https://docs.wagtail.org/en/stable/releases/5.0.html

  • Update testing for Wagtail 5.0 and Django 4.2.
  • Dropped support for Python 3.7.

Copy link
Contributor

@nickmoreton nickmoreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just one small change to look at.

Comment on lines +7 to +8
py{311}-dj{41}-wt{41,42,50}
py{311}-dj{42}-wt{50}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
py{311}-dj{41}-wt{41,42,50}
py{311}-dj{42}-wt{50}
py{311}-dj{41,42}-wt{41,42,50}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I thought Wagtail 4.1 and 4.2 are not Django 4.2 ready because of:
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also,
Can it be:

py{38,39,310}-dj{32,40}-wt{41,42}
py{38,39,310,311}-dj{41,42}-wt{41,42,50}

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we should also test Python versions 3.8-3.10 for Wagtail 5.0

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, Django 4.2 is indeed not compatible with Wagtail 4.1 and 4.2.
Changed to:

py{38,39,310}-dj{32,40,41}-wt{41,42}
py{311}-dj{32,41}-wt{41,42,50}
py{38,39,310,311}-dj{42}-wt{50}

Please let me know if I forgot a combination or if there's a more efficient way. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @katdom13 I think we've had too much tox in our lives 😅 py311 and dj32 won't work will it? I've caused you more confusion with my initial comment. You are right that wt50 is the only version that supports dj42.

Copy link
Contributor

@nickmoreton nickmoreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just one small change to look at.

Copy link
Contributor

@MrCordeiro MrCordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @katdom13 !

Please don't forget to update the docs as well 🙏
https://github.com/jazzband/wagtailmenus/blob/master/docs/source/index.rst

(I can also do it on the weekend if you don't have the time)

@katdom13
Copy link
Author

katdom13 commented Jun 9, 2023

Thanks for the PR, @katdom13 !

Please don't forget to update the docs as well 🙏 https://github.com/jazzband/wagtailmenus/blob/master/docs/source/index.rst

(I can also do it on the weekend if you don't have the time)

Hi @MrCordeiro ,
Thank you for the reminder,
I updated the version compatibility here:
https://github.com/jazzband/wagtailmenus/pull/457/files#diff-4eec6cec5f6fab1548b85433ea8ca81315ae165db4b7f84019f287df9015699f,

Please let me know if I forgot something.
Thanks!

@katdom13 katdom13 force-pushed the support/wagtail-50 branch 2 times, most recently from 2a524aa to 46754f4 Compare June 9, 2023 03:30
Copy link
Contributor

@MrCordeiro MrCordeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MrCordeiro MrCordeiro merged commit 32b8dcd into jazzband:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants