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

Draft: Wagtail 4.2 support #66

Merged
merged 5 commits into from
Mar 3, 2023

Conversation

katdom13
Copy link

@katdom13 katdom13 commented Feb 2, 2023

  • Updated tox testing and workflow actions
  • Removed support for Wagtail versions < 4.1
  • Suppress warnings
  • Code cleanup

All tests run OK

Found 19 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
...................
----------------------------------------------------------------------
Ran 19 tests in 0.920s

OK

@katdom13 katdom13 changed the title Wagtail 4.2 support Draft: Pre-release Wagtail 4.2 support Feb 2, 2023
Copy link
Member

@chosak chosak left a comment

Choose a reason for hiding this comment

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

Thank you @katdom13! We don't typically make packages releases for Wagtail release candidates but can definitely do this once 4.2 is out.

Along with this change we could probably also remove 4.0 from the tox/GitHub testing matrix - we should keep 4.1 as it's an LTS but explicitly testing 4.0 doesn't seem necessary. Any thoughts @willbarton?

@katdom13
Copy link
Author

katdom13 commented Feb 6, 2023

Hi @chosak and @willbarton ,
I have removed versions of Wagtail < 4.1 from the testing matrix.

Would you also like me to drop support for Wagtail < 4.1 altogether? This would also mean removing code inside conditionals (e.g. inside if WAGTAIL_VERSION >= (x, y)) according to [the guidelines]?(https://github.com/wagtail/wagtail/wiki/Python-Package-Maintenance-Guidelines#setuppy--setupcfg--pyprojecttoml)

Please note the EOL and support: https://endoflife.date/wagtail

Thank you!

@katdom13 katdom13 requested a review from chosak February 8, 2023 07:57
@katdom13
Copy link
Author

katdom13 commented Feb 8, 2023

Hi @chosak , @willbarton
This MR should drop versions of Wagtail < 4.1.
Thanks!

@katdom13 katdom13 changed the title Draft: Pre-release Wagtail 4.2 support Draft: Wagtail 4.2 support Feb 10, 2023
Copy link

@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.

LGTM. Just a couple of possible changes

@@ -12,7 +12,7 @@


if wagtail.VERSION < (3, 0): # pragma: nocover

Choose a reason for hiding this comment

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

Could this be removed and below you can then not need to use the CORE_BLOCKS and use the full string.

e.g. wagtail.blocks.field_block.CharBlock

Copy link
Author

Choose a reason for hiding this comment

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

Hi @nickmoreton ,
Good catch! This has been updated in the latest commit b2e4161
This should also resolve the one below.

from wagtail.core.models import Page, Site
from wagtail.tests.utils import WagtailTestUtils
from wagtail.models import Page, Site
from wagtail.test.utils import WagtailTestUtils

from wagtailinventory.models import PageBlock


if wagtail.VERSION < (3, 0): # pragma: nocover

Choose a reason for hiding this comment

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

Ditto

@katdom13 katdom13 requested review from nickmoreton and chosak and removed request for chosak and nickmoreton February 10, 2023 15:00
Copy link

@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.

LGTM 👍

@willbarton
Copy link
Member

@katdom13 for us, it would be preferable to maintain Wagtail 2.15 support a bit longer, as that's where we currently are (but are in the process of updating).

Our preference is to test against LTS releases, so that would be 2.15 and 4.1. Unfortunately that means also maintaining the if WAGTAIL_VERSION >= (x, y) tests for now.

@nickmoreton
Copy link

@katdom13 for us, it would be preferable to maintain Wagtail 2.15 support a bit longer, as that's where we currently are (but are in the process of updating).

Our preference is to test against LTS releases, so that would be 2.15 and 4.1. Unfortunately that means also maintaining the if WAGTAIL_VERSION >= (x, y) tests for now.

Thanks @willbarton

@katdom13 If you will have time to look at this that would be great but let me know if you would like me to pick it up? Thanks

@willbarton
Copy link
Member

@katdom13 @nickmoreton I can pick this up. We're in the midst of converting our projects to pyproject.toml too, so I'm going to pick up the commits here, do that conversion and we'll do a release based on that.

Thanks for your patience!

@willbarton
Copy link
Member

Thank you so much @katdom13 and @nickmoreton! These changes have been merged in #67 (fascinating that GitHub sees that and indicates this PR is merged now too!)

We'll get a release out with them shortly!

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.

4 participants