-
Notifications
You must be signed in to change notification settings - Fork 53
Version 2.0: write CTE queries with any model #117
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
Conversation
Both old and new syntax will be supported for some time. We want to retain these tests while v1 syntax is supported.
Avoid confusion around when a CTE is added to another query, which is done by `with_cte`.
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.
Pull Request Overview
This PR upgrades django-cte to version 2.0 by replacing the old With
/CTEManager
API with a new CTE
class and standalone with_cte
function, while deprecating and removing legacy manager/queryset workarounds and streamlining internals.
- Rename
With
toCTE
and deprecateCTEManager
/CTEQuerySet
- Introduce
with_cte(select=…)
function and update all call sites - Simplify internal compiler/jitmixin implementations and update docs/tests
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/test_v1/init.py | Add fixtures to suppress v1 deprecation warnings |
tests/test_recursive.py | Update imports from With to CTE and use with_cte |
tests/test_raw.py | Update raw CTE tests to use CTE and with_cte |
tests/test_manager.py | Remove deprecated manager tests and use with_cte |
tests/test_django.py | Drop old CTE manager tests and update to new API |
tests/test_cte.py | Switch from With to CTE and apply with_cte |
tests/test_combinators.py | Update union/intersect tests to use new API |
tests/models.py | Remove CTEQuerySet/CTEManager inheritance from models |
tests/init.py | Add ignore_v1_warnings contextmanager |
docs/index.md | Revise docs to reflect v2 syntax (CTE + with_cte ) |
django_cte/raw.py | Refactor raw_cte_sql inner classes to new style |
django_cte/query.py | Refactor CTEQuery, compilers, and query mixin logic |
django_cte/meta.py | Simplify super calls and class signatures |
django_cte/join.py | Remove redundant object base class |
django_cte/jitmixin.py | Add JITMixin and jit_mixin helpers |
django_cte/expressions.py | Remove legacy CTESubqueryResolver |
django_cte/cte.py | Implement CTE class, with_cte function, deprecations |
django_cte/_deprecated.py | Add backport of @deprecated decorator |
django_cte/init.py | Bump version to 2.0.0 and update __all__ |
.github/workflows/tests.yml | Rename Postgres job and add SQLite test step |
Comments suppressed due to low confidence (4)
tests/test_django.py:18
SkipTest
is not imported in this file. Addfrom unittest import SkipTest
(or usepytest.skip()
) at the top.
raise SkipTest("feature added in Django 4.2")
django_cte/cte.py:23
- The docstring refers to
name
andmaterialized
parameters, butwith_cte
does not accept those. Update the docstring to match the actual signature.
:param name: Optional name parameter for the CTE (default: "cte").
tests/test_recursive.py:255
- Extra closing parenthesis causes a syntax error. Remove the redundant
)
after the.filter(...)
call.
).filter(_exclude_leaves=True))
.github/workflows/tests.yml:89
- [nitpick] This SQLite test step reuses
DB_SETTINGS
from the previous step. Consider explicitly clearing or settingDB_SETTINGS
for SQLite to ensure the correct database configuration.
- name: Run tests on SQLite
Takes one or more CTE queries and a `select` query to which the CTEs will be added.
More inline with how Query.get_compiler works on Django 4.2+
This commit may be easier to view by ignoring whitespace changes.
Play nice with other custom Query implementations
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.
Looks good. Just a couple of simple typo fixes.
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
self._with_ctes = [] | ||
_with_ctes = () |
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.
Can you elaborate on the motivation for switching from a list to a tuple?
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.
Yes, it follows the same pattern as combined_queries
(Query
attribute), which is also a tuple.
Co-authored-by: Graham Herceg <[email protected]>
Key changes:
With
is renamed toCTE
.With
has been deprecated and will be removed in a future version of django-cte.with_cte
moved fromCTEQuerySet
method to a stand-alone function.CTEManager
andCTEQuerySet
are deprecated and should be removed from code that uses them since they are no longer necessary. They will be removed in a future version of django-cte.Internally, the library has been updated to simplify the code and remove workarounds for old/unsupported versions of Django.