Update test scope to include django 5.1, update pre-commit hooks #683

kwongtn · 2024-12-30T12:07:44Z

Description

Update test scope & pre-commit hooks.
Replaced django-mptt with django-tree-queries

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Summary by Sourcery

Extend support to Django 5.1 and update pre-commit hooks.

Enhancements:

Updated Django to 5.1.
Updated pre-commit hooks.

Tests:

Updated tests to account for Django 5.1 changes.

Signed-off-by: Kwong Tung Nan <[email protected]>

sourcery-ai · 2024-12-30T12:07:48Z

Reviewer's Guide by Sourcery

This pull request updates the test scope to include Django 5.1 and adjusts the code to accommodate changes in Django 5.1, specifically the removal of support for 'index_together' in meta classes. It also updates the pre-commit hooks and modifies the test configurations to ensure compatibility with the new Django version.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Adaptation to Django 5.1 changes in model meta options.	Imported necessary modules for Django 5.1 compatibility. Defined a custom MPTTModelBase class to handle tree model registration. Added logic to manage unique_together and index_together attributes for tree models. Ensured tree manager is added to models if not present.	`tests/relay/mptt/models.py`
Update test configurations to include Django 5.1.	Added Django 5.1 to the list of versions in the test matrix. Updated Python version compatibility for Django 4.2, 5.0, and 5.1. Included and excluded specific Python versions for different Django versions.	`.github/workflows/tests.yml`
Update pre-commit hook version for ruff.	Updated ruff-pre-commit hook from version v0.8.3 to v0.8.4.	`.pre-commit-config.yaml`
Exclude specific test file from pyright type checking.	Added 'tests/relay/mptt/models.py' to the exclude list in pyproject.toml.	`pyproject.toml`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @kwongtn - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider keeping pyright type checking enabled for tests/relay/mptt/models.py if possible, as it could help catch potential issues earlier. If there are specific reasons for excluding it, perhaps add a comment explaining why.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2024-12-30T12:08:39Z

pyproject.toml

@@ -177,7 +177,7 @@ max-nested-blocks = 7
 [tool.pyright]
 pythonVersion = "3.9"
 useLibraryCodeForTypes = true
-exclude = [".venv", "**/migrations", "dist", "docs"]
+exclude = [".venv", "**/migrations", "dist", "docs", "tests/relay/mptt/models.py"]


question (testing): Could you explain why this specific test file needs to be excluded from type checking?

Excluding test files from type checking might hide potential issues. If there's a specific reason for this exclusion, it would be helpful to document it.

Do we need to exclude the whole file for this? Can we just # type: ignore or cast in the lines that need that?

Yes, using # type: ignore or cast on the specific lines causing type errors would be a better approach than excluding the entire file. This way, we maintain type checking for the rest of the test file and only bypass it where absolutely necessary. Could you please update the PR with these more granular type ignores/casts? This will help us catch any potential type errors in the other parts of the test file. Thanks!

codecov-commenter · 2024-12-30T12:10:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.17%. Comparing base (48b54fd) to head (392676c).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #683   +/-   ##
=======================================
  Coverage   89.17%   89.17%           
=======================================
  Files          41       41           
  Lines        3778     3778           
=======================================
  Hits         3369     3369           
  Misses        409      409

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bellini666

Ty for this! :)

Left a couple of suggestions/questions

.github/workflows/tests.yml

bellini666 · 2024-12-30T17:01:57Z

.github/workflows/tests.yml

+        include:
+          # Django 4.2 only supports python 3.8-3.12
+          - django-version: 4.2.*
+            python-version: '3.8'
+            mode: std
+          - django-version: 4.2.*
+            python-version: '3.8'
+            mode: geos
        exclude:
-          # Django 5.0 only supports python 3.10+
+          # Django 4.2 only supports python 3.8-3.12
+          - django-version: 4.2.*
+            python-version: '3.13'


question: How does this works when we have both include and exclude?

As far as I know, they are not mutually exclusive. And according to docs

All include combinations are processed after exclude. This allows you to use include to add back combinations that were previously excluded.

Exclude works by excluding anything that matches the declared params.
For example as of the above, it excludes Django 4.2 and Python 3.13, no matter if it is geo or standard.

As for include, I added it in as it'll be redundant to add Python 3.8 to the matrix, then exclude all except Django 4.2 😅

bellini666 · 2024-12-30T17:02:26Z

pyproject.toml

@@ -177,7 +177,7 @@ max-nested-blocks = 7
 [tool.pyright]
 pythonVersion = "3.9"
 useLibraryCodeForTypes = true
-exclude = [".venv", "**/migrations", "dist", "docs"]
+exclude = [".venv", "**/migrations", "dist", "docs", "tests/relay/mptt/models.py"]


Do we need to exclude the whole file for this? Can we just # type: ignore or cast in the lines that need that?

bellini666 · 2024-12-30T17:03:39Z

tests/relay/mptt/models.py


+if django.VERSION >= (5, 1):


question: why is this needed?

Can you add a comment with the explanation, and also pointing out where this code came from?

Turns out its not needed anymore 🥳
According to django mptt repo, django-mptt is now unmaintained. It recommends using django-tree-queries instead.

Hence I tried using it, and guess what all tests pass 😁
Will be deprecating django-mptt and removing references in an upcoming PR

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

Signed-off-by: Kwong Tung Nan <[email protected]>

bellini666

Ty! :)

kwongtn added 4 commits December 30, 2024 19:11

Update test versions

7b0d046

Signed-off-by: Kwong Tung Nan <[email protected]>

Autoupdate pre-commit hooks

9834dc3

Signed-off-by: Kwong Tung Nan <[email protected]>

Add compatibility layer for django 5.1

be5d63e

Signed-off-by: Kwong Tung Nan <[email protected]>

Ignore mptt test models

de8a3b6

Signed-off-by: Kwong Tung Nan <[email protected]>

sourcery-ai bot reviewed Dec 30, 2024

View reviewed changes

bellini666 reviewed Dec 30, 2024

View reviewed changes

kwongtn and others added 5 commits December 31, 2024 10:40

Update .github/workflows/tests.yml

4063dea

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

Update .github/workflows/tests.yml

d97e2c3

Co-authored-by: Thiago Bellini Ribeiro <[email protected]>

Attempt to use django-tree-node to replace django-mptt

f219f16

Rename TCH002 to TC002

16c1792

Signed-off-by: Kwong Tung Nan <[email protected]>

Remove pyright exclusion

392676c

Signed-off-by: Kwong Tung Nan <[email protected]>

kwongtn requested a review from bellini666 January 2, 2025 02:34

This was referenced Jan 2, 2025

test: Replace django mptt with django tree queries for tests #684

Merged

[Draft] Enable psycopg v3 tests #685

Draft

bellini666 approved these changes Jan 3, 2025

View reviewed changes

bellini666 merged commit 1ca8aaa into strawberry-graphql:main Jan 3, 2025
27 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update test scope to include django 5.1, update pre-commit hooks #683

Update test scope to include django 5.1, update pre-commit hooks #683

kwongtn commented Dec 30, 2024 •

edited

Loading

sourcery-ai bot commented Dec 30, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

sourcery-ai bot Dec 30, 2024

bellini666 Dec 30, 2024

sourcery-ai bot Dec 30, 2024

codecov-commenter commented Dec 30, 2024 •

edited

Loading

bellini666 left a comment

bellini666 Dec 30, 2024

kwongtn Jan 2, 2025

bellini666 Dec 30, 2024

bellini666 Dec 30, 2024

kwongtn Jan 2, 2025

bellini666 left a comment

Update test scope to include django 5.1, update pre-commit hooks #683

Update test scope to include django 5.1, update pre-commit hooks #683

Conversation

kwongtn commented Dec 30, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

Summary by Sourcery

sourcery-ai bot commented Dec 30, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Dec 30, 2024

Choose a reason for hiding this comment

bellini666 Dec 30, 2024

Choose a reason for hiding this comment

sourcery-ai bot Dec 30, 2024

Choose a reason for hiding this comment

codecov-commenter commented Dec 30, 2024 • edited Loading

Codecov Report

bellini666 left a comment

Choose a reason for hiding this comment

bellini666 Dec 30, 2024

Choose a reason for hiding this comment

kwongtn Jan 2, 2025

Choose a reason for hiding this comment

bellini666 Dec 30, 2024

Choose a reason for hiding this comment

bellini666 Dec 30, 2024

Choose a reason for hiding this comment

kwongtn Jan 2, 2025

Choose a reason for hiding this comment

bellini666 left a comment

Choose a reason for hiding this comment

kwongtn commented Dec 30, 2024 •

edited

Loading

sourcery-ai bot commented Dec 30, 2024 •

edited

Loading

codecov-commenter commented Dec 30, 2024 •

edited

Loading