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

Update python and django versions to run against #30

Merged
merged 5 commits into from
Nov 28, 2023
Merged

Conversation

gherceg
Copy link
Contributor

@gherceg gherceg commented Nov 20, 2023

Expand tests to run against more versions of python, and only run against LTS releases of django and django 5.0 pre-release.

In Django 4.2, the behavior of __invert__ on Q objects was optimized
to toggle the negated attribute on the existing Q rather than add
an AND operator that wraps the inner clause.

On Django 4.2:
> from django.db import models
> ~(models.Q(test=1) | models.Q(test=2))
<Q: (NOT (OR: ('test', 1), ('test', 2)))>
> models.Q(models.Q(test=1) | models.Q(test=2), _negated=True)
<Q: (NOT (AND: (OR: ('test', 1), ('test', 2))))>

The optimized version is functionally the same, but creates an
unnecessary migration. This commit changes the syntax used when
defining the constraint to preserve the current format defined
in the migration.
Copy link
Contributor

@millerdev millerdev left a comment

Choose a reason for hiding this comment

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

One small suggestion for the model/migration change. Otherwise looks good.

@@ -216,10 +216,11 @@ class Meta:
constraints = [
models.CheckConstraint(
name="field_audit_auditevent_chk_create_or_delete_or_bootstrap",
check=~(
check=models.Q(
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than changing this model code, could we change the migration code to use the ~ syntax?

check=models.Q(
models.Q(
models.Q(('is_create', True), ('is_delete', True)),
models.Q(('is_bootstrap', True), ('is_create', True)),
models.Q(('is_bootstrap', True), ('is_delete', True)),
_connector='OR',
),
_negated=True,
),

Should verify that it does not change the migration SQL, and also that it does not cause Django 3.2 to want to create a new migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran sqlmigrate on the existing migration and the autogenerated migration by Django 4.2 to compare.

Old

CHECK (NOT ((("is_create" AND "is_delete") OR ("is_bootstrap" AND "is_create") OR ("is_bootstrap" AND "is_delete"))))

New

CHECK (NOT (("is_create" AND "is_delete") OR ("is_bootstrap" AND "is_create") OR ("is_bootstrap" AND "is_delete")))

So the difference is just an extra parenthesis around the inner condition of the NOT, but is that enough of a difference to warrant not changing the existing migration file itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Semantically that is no different. I think it would be fine to change the migration file if it does not cause Django 3.2 to want to create a new migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. See 0bb2fc9. I tested that makemigrations on both 3.2 and 4.2 does not result in a new migration being created.

@dannyroberts
Copy link
Member

dannyroberts commented Nov 28, 2023

I updated the branch rules to require 3.9, django==3.2.*, false and 3.9, django==4.2.*, false. This is conceptually equivalent to what we had before, but the naming of these tests changed slightly Graham pointed out to me that it was blocking.

@gherceg gherceg merged commit 8935d28 into main Nov 28, 2023
10 checks passed
@gherceg gherceg deleted the gh/update-versions branch November 28, 2023 22:00
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