#29799 closed New feature (fixed)
Allow registration and unregistration of lookups per field instances.
Reported by: | Simon Charette | Owned by: | AllenJonathan |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | GSoC |
Cc: | Can Sarıgöl, Himanshu Balasamanta, Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Under certain circumstances it might be useful to register or unregister model field lookups on a per instance basis instead of a per class basis.
For example, it could be useful to disable costly lookups such as contains
and icontains
on some large table model CharField
s without defining them using a CharField
subclass to prevent them from being used in production. Similarly you might want to register or override a specific lookup for a third party app model field that doesn't allow swapping.
This was initially mentioned in #16187 which introduced this new lookup registration system.
https://code.djangoproject.com/ticket/16187#comment:14
Having the ability to hook a custom lookup to a given field instance would be really nice to have...
https://code.djangoproject.com/ticket/16187#comment:22
Add a way to register lookups to fields (I think either instances or classes is wanted behaviour).
But not discussed afterwhile. I believe class based registration was chosen because it was easier to implement but I agree that being able to register custom lookups per field instance would be good feature addition.
Change History (17)
comment:2 by , 6 years ago
Cc: | added |
---|
comment:4 by , 5 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:5 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 3 years ago
@carltongibson , @charettes . I wanted to clarify exactly what all the scope of the ticket covers.
As per my understanding,
- Enable registering and removal of a lookup for instances as it is with classes.
- Enable lookups registered for the class to be unregistered (and maybe re-registered?) for the instances of that class.
Description of the functions would be as follows
- get_lookups(selforclass)
- when called by a field instance, returns all class and instance lookups currently registered for that field instance
- when called by a class, returns all class lookups currently registered for that class
- register_lookup(selforclass, lookup, lookup_name)
- when called by a field instance, registers the lookup for that field instance
- when called by a class, registers the lookup for that class
- unregister_lookup(selforclass, lookup_name)
- when called by a field instance,
- if the lookup is already registered with the instance, it gets removed,
- if the lookup is registered with the instance class, that particular instance is exempted
- else throws error
- when called by a class, if the lookup is already registered with the class, it is removed. Else error is thrown.
Doubts:
- Should class lookups that are unregistered for an instance be re-registered using register_lookup() itself?
- Should there be a method to specifically get lookups that were registered for the instance and not the class?
comment:7 by , 3 years ago
Cc: | removed |
---|
follow-up: 9 comment:8 by , 3 years ago
Hi Himanshu.
The rough outline looks right. There are various variations but, I'd like to add a custom lookup just for this model — the model being the bearer of field instances; I'd like to disable this lookup on this model, and so on.
The specific behaviours are hard to say definitively one way or the other until we get to the review stage. Looking at test cases, and seeing how the API looks at that point is much easier to speak clearly to.
(For example, I'm not sure what to say about trying to unregister a non-registered lookup on a class... — we could error, or we could pass
: the end effect is that the lookup is not registered on the class... — My query would be why an error? 🤔 — it's the kind of detail we can settle with multiple sets of eyes at review time.)
I know you're looking at this targeting GSoC, so for the the sake of a proposal, noting the areas to be decided is OK. Showing that you've considered points already raised is important — make sure you looked at Simon's comment on your draft PR — I'm sure you did. Plus #33626 and the PR there you've made are all good signs — that's on my list to review next week.
I hope that helps. 🙂
comment:9 by , 3 years ago
Replying to Carlton Gibson:
The rough outline looks right. There are various variations but, I'd like to add a custom lookup just for this model — the model being the bearer of field instances; I'd like to disable this lookup on this model, and so on.
Agreed.👍 It would be more user friendly.
So, along with register_lookup(self,lookup,lookup_name), I shall add a Model.register_lookup(field_name : str, lookup : Lookup, lookup_name : str) too.
I know you're looking at this targeting GSoC, so for the the sake of a proposal, noting the areas to be decided is OK. Showing that you've considered points already raised is important — make sure you looked at Simon's comment on your draft PR — I'm sure you did. Plus #33626 and the PR there you've made are all good signs — that's on my list to review next week.
I hope that helps. 🙂
Thank You!
comment:10 by , 3 years ago
Cc: | added |
---|
comment:11 by , 3 years ago
Cc: | added |
---|---|
Has patch: | unset |
Keywords: | GSoC added |
Needs documentation: | unset |
Owner: | changed from | to
Patch needs improvement: | unset |
comment:14 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:15 by , 2 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |