Repository navigation
Conversation
84a7454 to
3b915a5
Compare
d3bcd49 to
2968b3a
Compare
|
I didn't mention above, because I assumed it was common knowledge. But, my intention here is try and make Wagtail projects a little less 'special' in terms of dev approach to other Django projects. I consider myself to be a pretty hardcore 'Django principles' guy, and even I've avoided queryset customisation for child models because of this class of issue. Child object filtering logic usually ends up as cautiously-written methods on the parent model instead, with conditional logic to account for preview behaviour. This then sets an unhealthy precedent for the project - Less django-familiar devs then come to the project and think that's the preferred pattern for everything, and the project slowly merges away from queryset customisation, even where draft preview isn't a concern. Ultimately, Wagtail being based on Django is one of its greatest strengths - A lot of folks love Django because of it's clear and consistant patterns, and AI generally understands these same patterns pretty well. I want to make queryset customisation the standard for all Django projects - including ones using Wagtail. |
Co-authored-by: Andy Babic <ababic@users.noreply.github.com>
Co-authored-by: Andy Babic <ababic@users.noreply.github.com>
Co-authored-by: Andy Babic <ababic@users.noreply.github.com>
2968b3a to
80cd62a
Compare
Implements #209
Description
Allows registering of methods on a models manager's default
QuerySetas compatible withFakeQuerySetclasses via a new@fakequeryset_compatibledecorator - so that they become available in draft/preview/in-memory-only contexts.When used, assembled in-memory classes take on the name
FakeModelNameQuerySet, and have the decorated methods copied over to them. The methods will then be available to use on in-memory versions of those querysets, AND fake querysets returned by parent -> child relationship access.The generated classes are cached to avoid repeat effort, and are true
FakeQuerySetsubclasses, so any in-project code that doesisinstance(queryset, FakeQuerySet)will continue to work with these changes in place.The decorator supports an
as_nameoption, which allows developers to write alternative implementations of methods on querysets specifically for in-memory usage (where they live alongside the original implementation). This is useful in cases where developers want the 'regular' querysets to continue to use the ORM implementation, but are happy to add a pure-Python version for the in-memory representation case, where the options would be:selfto serve as a stub/no-opAttempts to override methods that
FakeQuerySetalready implements will raise a clear warning, and the registered version of the method will be ignored.Additional changes
It's clear from issue reports that there is a lot of confusion when developers see a bare
AttributeErrorwhen trying to use custom queryset methods in a draft/preview context without an explanation of why it's occurring. So, this PR introducesQuerySetMethodOrAttributeUnavailableErrorto better explain why it is happening, and also proposes a solution. It's anAttributeErrorsubclass, so any existing projects attempting to catch/handle the original exception will still work.Design justifications
Why not just force devs to define a custom FakeQuerySet class, and register that somehow?
While this might be beneficial from a clarity / typing perspective, it comes with complication and development overhead, e.g.
The simple decorator approach solves all this by:
Plus,
django-modelclusteris really a 'tool choice' of Wagtail. I think forcing a full understanding of FakeQuerySet on developers and pushing them toward class replication / duplication is a bad idea generally - This isn't a common enough problem in projects to make devs completely rethink their approach to code definition. Being able to decorate existing code in-place is a low-footprint 'Get out of jail free card' - just for when you need it.Why log a warning when method name clashes are detected instead of raising a hard exception?
This could add unnecessary friction to the upgrade process should the
FakeQuerySetAPI be updated to support more of Django's QuerySet API in future. Release management is complicated enough without having to worry about breaking live-running projects that are currently using their own, native implementation.Exciting thoughts
QuerySetMethodOrAttributeUnavailableErrorin the first-place - all without polluting FakeQuerySets's generic API.AI usage
Like many devs, I use AI as part of my everyday development process - but, never as a shortcut to thinking things through. In this case, OpenAI's Codex 5.3 model was used (via Cursor) for scaffolding the changes based on the issue description I put together. I then iterated on that implementation to add the method-name conflict handling and other tweaks. The idea itself, decisions on naming/messaging, inheritance-scenario tests etc were all my own - as is this PR description.