tidal: respect search_limit config option in candidates()#6781
Conversation
TidalPlugin.candidates() and item_candidates() called search_albums_by_query() and search_tracks_by_query() without passing any limit, ignoring the search_limit config value inherited from MetadataSourcePlugin. Add an optional limit parameter to both query methods and slice the result list before fetching full records, matching the pattern used by other direct-implementation plugins. Fixes beetbox#6770 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6781 +/- ##
=======================================
Coverage 74.84% 74.85%
=======================================
Files 163 163
Lines 20949 20946 -3
Branches 3300 3297 -3
=======================================
- Hits 15680 15679 -1
+ Misses 4511 4510 -1
+ Partials 758 757 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for already taking a look here!
We should be able to resolve all search limit based logic within the candidates and item_candidates function. I don't really see the need for the extensive drilling.
The search_tracks_by_query and search_albums_by_query functions are returning iterators. We should be able to break them early and limit the number of requests that way.
Additionally the search_limit currently does not respect the _album_queries and _item_queries loop. We might want to have a round robin approach here.
I will have a more detailed look next week.
|
I've added it to my fork using a slightly simpler solution 626963d |
Instead of drilling a limit parameter into search_albums_by_query and search_tracks_by_query, handle the cap entirely inside candidates() and item_candidates() by consuming each query's iterator in a round-robin loop and stopping as soon as search_limit results have been collected. This keeps the query functions pure and correctly interleaves results across multiple queries (e.g. multiple artist/album combinations from _album_queries) rather than exhausting one query before moving to the next. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
hi @semohr thank you for the feedback! I completely agree, so moved the limit logic entirely into candidates() and item_candidates() using a round-robin loop that early-breaks across query iterators, so search_albums_by_query and search_tracks_by_query stay clean. Also added a test covering the multi-query interleaving behaviour. |
- Wrap query iterables with iter() so next() satisfies mypy's SupportsNext constraint - Shorten two docstrings that exceeded the 88-char line limit - Reformat test file to satisfy ruff Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use cached_property for search_limit and replace manual round-robin loops with itertools.chain.from_iterable, zip_longest, and islice.
Use None as the zip_longest fill value so mypy can narrow candidate types instead of inferring object from an object() sentinel.
|
|
||
| for query in self._item_queries(item): | ||
| candidates += self.search_tracks_by_query(query) | ||
| candidates = list( |
There was a problem hiding this comment.
Lets make the round robin logic a helper function. Should make the flow here more readable and should allow to dedupe some code.
| ( | ||
| candidate | ||
| for candidate in itertools.chain.from_iterable( | ||
| itertools.zip_longest( |
There was a problem hiding this comment.
What is zip_longest here for? Also, you can use filter(None, ... instead of checking for None values.
Fixes #6770
Problem
TidalPluginextendsMetadataSourcePlugin, which sets a defaultsearch_limit: 5config value. However,candidates()anditem_candidates()calledsearch_albums_by_query()andsearch_tracks_by_query()without any limit, so the config option was silently ignored.Fix
limit: int | None = Noneparameter tosearch_albums_by_query()— slicesalbum_idsbefore passing tosearch_albums_by_ids()limit: int | None = Noneparameter tosearch_tracks_by_query()— slices the track relationships list before iteratingcandidates()readsself.config["search_limit"].get(int)and passes it throughitem_candidates()does the sameThe fix is intentionally minimal — no changes to
api.pyor the Tidal HTTP request; results are simply sliced client-side, consistent with how other direct-implementation plugins cap their results.Tests
Added
TestSearchLimitwith two tests:test_candidates_respects_search_limit— setssearch_limit: 1, mockssearch_resultsreturning 2 album IDs, asserts only 1 candidate is returnedtest_item_candidates_respects_search_limit— same pattern for tracks