django
django copied to clipboard
django
WIP #29799 -- Per-Field-instance lookups
This class seems to be the best way to differentiate class or instance calls (though a little less readable). I am unclear about whether tests should be written for this class separately (since this would be tested indirectly when other methods are tested).
@AllenJonathan Thanks :+1: I don't think it's worth adding only classorinstancemethod. It's difficult to judge how useful it is without remaining elements. Can you add other pieces (even a draft) that we could discuss the implementation details?
Please ignore the linter errors (using a different device and so facing issues with pre-commit). register_lookup and _unregister_lookup seems to working as it should be.
One issue that I'm facing is that all Field instances are wrapped in DeferredAttribute class. Which make the syntax like this: Model.name.field.register_lookup(Lookup) instead of: Model.name.register_lookup(Lookup).
Please ignore the linter errors (using a different device and so facing issues with pre-commit).
You don't need to use pre-commit, you can call black and isort on changed files.
@AllenJonathan Thanks 👍 This PR is mainly based on charettes@ccc58bf. We need much more: new context processors for registering lookups per field/instance, support reverse relations, tests with all three layers (class, instance, field) etc.
Any related pull request could be helpful. Also, I don't understand how context processors would come into play here, As far as I know, context_processors are used to render data in templates.
Also, I don't understand how context processors would come into play here, As far as I know, context_processors are used to render data in templates.
Context processors have many uses, I'm thinking about sth similar to the django.test.utils.register_lookup:
https://github.com/django/django/blob/e59d1ff5627cf9cc551e160d65858a37693de4ed/django/test/utils.py#L989-L1001
Also, I don't understand how context processors would come into play here, As far as I know, context_processors are used to render data in templates.
Context processors have many uses, I'm thinking about sth similar to the
django.test.utils.register_lookup:https://github.com/django/django/blob/e59d1ff5627cf9cc551e160d65858a37693de4ed/django/test/utils.py#L989-L1001
So as far as I understand, a context processor that would temporarily register (Field/instance) lookup should be added. In this way, testing would be simplified as the lookups would be unregistered automatically. So the use case would be for testing only. Am I right?
So as far as I understand, a context processor that would temporarily register (Field/instance) lookup should be added. In this way, testing would be simplified as the lookups would be unregistered automatically. So the use case would be for testing only. Am I right?
Yes.
- Tests are written for both
Fieldclass and instances. The basicfiltertests seem to work. Let me know about other corner cases that can added.
One issue that I'm facing is that all Field instances are wrapped in DeferredAttribute class. Which make the syntax like this: Model.name.field.register_lookup(Lookup) instead of: Model.name.register_lookup(Lookup).
-
This could be solved easily by adding the necessary functions in
DeferredAttributeas class variables (functions). But I'm unsure whether to do so. Or we could also make the syntax asModel.name.field.register_lookup(Lookup). Let me know which should be done -
Should an error be given if an unregistered lookup is unregistered. If yes what error could it be?
-
About context processors
register_lookuppresent in thedjango.test.utils. I feel no change needs to be done here. As theregister_lookupautomatically differentiates class and instances and you can see in the tests that it works perfectly fine. Let me know why exactly a new context processor should be implemented.
I would start now on the documentation changes. Maybe reverse relations could be done after these things are completed and resolved? But some insight into what needs to be done about reverse relations would be very helpful.
@felixxm I've made some changes. Please look into it. Let me know if there is anything else to add(or change) here. I'm going to start adding documentation changes.
We already got a document pattern of to access model fields so it seems a bit odd to me that we add all of this code to support an uncommon access pattern anyway.
Agreed, let's simplify this :+1:
Agreed, let's simplify this 👍
Ok. I'll mention the pattern to access model fields in the RegisterLookupMixin docs. I too felt that simplifying the syntax using proxy methods would be too much ugly code in several places,
I was wondering what are the benefits of performing any queries in
RegisterLookupTests.Couldn't we simply use
get_lookupto assert the registration works fine instead of hitting the database? It seems it would also make the tests more explicit.
This could be done and it would be fine. I've been hitting the database to ensure all the code works from the user level. I think there's no harm in testing by just using get_lookup either. But do let me know what to do.
For what it's worth I'm not entirely convinced that adding the
Model.field.register_lookupsugar is actually worth all the boilerplate it requires.We already got a document pattern of to access model fields so it seems a bit odd to me that we add all of this code to support an uncommon access pattern anyway.
Just my 2 cents.
Done👍
Couldn't we simply use get_lookup to assert the registration works fine instead of hitting the database? It seems it would also make the tests more explicit.
Agreed, let's switch to SimpleTestCase and get_lookups() assertions, without actually hitting the database.
I think this PR is completed. Is it ok to open this PR to merge?
I will check it next week. Can you explore more and maybe produce djangobench overview (as suggested by Simon)?
"It could be worth exploring as well as producing djangobench overview of the impact of this change on the time spent resolving lookups."
"Does the usage of functools.partial and the extra function call and dict lookup have a significant impact on query creation time?"
I've run djangobench on my cloned local django repository. I've compared the ticket_29799 branch with the main branch.
Here are the results.
qs_filter_chaining, query_aggregate, query_annotate, query_complex_filter, query_exists, and query_values have shown significant difference (slower than before). query_delete, query_filter and query_update have significantly changes (faster than before).
I used django-asv to compare this branch and the current main branch and there is no significant difference, see results :chart_with_upwards_trend: .
I squashed commits and rebased after e64919ae54933ef4840f0e27e51d9fcfd55ecf4b.