Filter an existing django queryset #168
Conversation
|
Can you please rebase it upon master and also describe the use case of this? |
andriilahuta
force-pushed
the
filter_queryset
branch
from
168cd5a to
63426aa
Compare
|
@safwanrahman it can be used when implementing a custom |
django_elasticsearch_dsl/search.py
Outdated
| """ | ||
|
|
||
| qs = _get_queryset(klass) |
There was a problem hiding this comment.
I am not comfortable to use a private method _get_queryset as it is not documented anywhere.
I think it is better to use self._model._default_manager for getting the default manager as the documentation recommend it.
|
@laevilgenius Sorry for the too late reply. I have one issue with the PR. Are you able to fix it? |
| @@ -13,12 +13,17 @@ def _clone(self): | |||
| s._model = self._model | |||
| return s | |||
|
|
|||
| def to_queryset(self, keep_order=True): | |||
| def filter_queryset(self, queryset, keep_search_order=True): | |||
There was a problem hiding this comment.
This should be a internal function so it should start with _
There was a problem hiding this comment.
I actually meant it to be public, so I could do something like this:
class SearchQuerySet(QuerySet):
def from_elastic(self, search):
return search.filter_queryset(self)Could you explain your reasoning?
There was a problem hiding this comment.
Interesting. I did not think about it before. Let me think about it so I can suggest a better approach.
There was a problem hiding this comment.
I think to_queryset should accept a queryset as optional parameter. So if queryset is passed, it will use that queryset. otherwise call the get_queryset method to get the queryset
@laevilgenius what do you think?
There was a problem hiding this comment.
Should we keep to_queryset backwards compatible? I'm thinking def to_queryset(self, qs=None, keep_order=True) vs def to_queryset(self, keep_order=True, qs=None).
|
I think there should be a Can you add a documentation about that also? |
| @@ -13,12 +13,17 @@ def _clone(self): | |||
| s._model = self._model | |||
| return s | |||
|
|
|||
| def to_queryset(self, keep_order=True): | |||
| def filter_queryset(self, queryset, keep_search_order=True): | |||
There was a problem hiding this comment.
Thanks for the update. As we already have get_queryset method, you can make the filter there. So we do not need another method for filter_queryset
|
|
||
| return queryset | ||
|
|
||
| def _get_queryset(self): |
There was a problem hiding this comment.
You can make this api public!
There was a problem hiding this comment.
I made it private so it would be less confusing with IDE hinting for example, as we have public methods that are mostly related to filtering and search object manipulation. What would be the public usage of get_queryset?
|
I think we can merge this PR as it is now. Sorry for the late in reviewing, better late than never! Thanks @andriilahuta |
Slight modification of
to_querysetmethod to allow filtering of existing django querysets.