Add filter_by_search_query support to get_movies, get_episodes and get_tv_shows. - #21
Add filter_by_search_query support to get_movies, get_episodes and get_tv_shows.#21anishsane wants to merge 1 commit into
Conversation
| @staticmethod | ||
| def _filter_by_search_query(original_result, search_query=None, result_type=None, score_cutoff=100): |
There was a problem hiding this comment.
Any reason we use a static method here as opposed to a separate method?
There was a problem hiding this comment.
I also prefer having a separate search method unless there's a good reason to go this route.
There was a problem hiding this comment.
It is a helper function. Hence prefixed by an _
I wanted to keep it under the Kodi class. So, not declared it as a global function.
But it did not need the self parameter. So, marked it as a static function.
If you prefer, I can make it a regular function and ignore the self parameter.
It will look like
- def _filter_by_search_query(original_result, search_query=None, result_type=None, score_cutoff=100):
+ def _filter_by_search_query(self, original_result, search_query=None, result_type=None, score_cutoff=100):
# .... existing code here
...
...
- return Kodi._filter_by_search_query(results, search_query, 'movies', score_cutoff)
+ return self._filter_by_search_query(results, search_query, 'movies', score_cutoff)
# etc
There was a problem hiding this comment.
I have updated the code as per your suggestion. Let me know if this is better.
There was a problem hiding this comment.
I think you missed the point. It's not about making this non-static (as you correctly mentioned, it doesn't use self), but rather adding a new, non-static, method, for search, rather than piggybacking on the existing ones.
There was a problem hiding this comment.
I have updated the PR. Replaced my earlier logic entirely. Please take a look.
There was a problem hiding this comment.
Hi,
Do you have any further suggestions for the PR?
Once this PR gets submitted, I can update my home_assistant/core PR accordingly.
There was a problem hiding this comment.
Sorry for the delay, will try to get to it and release a version in the next few days
There was a problem hiding this comment.
Hi,
Do you have any further suggestions for the PR?
Once this PR gets submitted, I can update my home_assistant/core PR accordingly.
|
|
||
| async def search_media(self, media_type, search_query=None, score_cutoff=80, **kwargs): | ||
| """ Filter the original result by the search query. """ | ||
| result = None |
There was a problem hiding this comment.
Since we're filtering, rather than searching, wouldn't it make sense to pass in the results instead?
There was a problem hiding this comment.
Sorry, I didn't update the comment earlier. Fixed it in a separate commit.
The results = None was never returned from the subsequent code. We either get the result from the corresponding get_* function and return filtered results, or in the default case, we return [] directly. 'None' was never returned by the function. But I have updated it to avoid confusion for the next person reading the code.
There was a problem hiding this comment.
I think you misunderstood my comment - we're doing pure filtering, there's no real "searching going on". Given than, wouldn't it make more sense to pass in the results from the outside? That way the filtering code doesn't need to know the different media types and how to get them. That way the filtering code automatically supports any media type, including potential future ones.
There was a problem hiding this comment.
Thank you for explaining. I have updated the code.
I have rebased and cleaned up the commit history to use only the filter_media function.
This PR will keep the discussion of earlier implementations.
This function takes 2 arguments: media and search_query. `media` is a list of movies/tv shows/episodes etc. This supplied media is filtered against the search query by a fuzzy match logic. This functionality will be used by Home Assistant for the 'Search and play' intent. The caller of this function should first get the complete list of media using the individual get_<media_type> functions. Then they should call the filter_media to filter based on the search_query. In addition to search_query, the user can also provide a parameter score_cutoff % (defaults to 80%) to be used as a threshold for fuzzy filtering.
3 participants
When calling these functions, the user can supply the optional string parameter - search_query.
If this parameter is provided, the results are filtered against the search_query parameter using a fuzzy search. This functionality will be used by Home Assistant for 'Search and Play' intent.
In addition to search_query, the user can also provide a parameter score_cutoff % (defaults to 80%) to be used as a threshold for fuzzy filtering.