fix: take every parent podcast out of the library, not every second one - #305
Open
mkb79 wants to merge 6 commits into
Open
fix: take every parent podcast out of the library, not every second one#305mkb79 wants to merge 6 commits into
mkb79 wants to merge 6 commits into
Conversation
Resolving podcasts replaces each parent with its episodes, and the parents are then removed. They were removed one at a time from the very list being walked, so each removal moved the rest along under the walk and the next parent was stepped over. It only shows when parents sit next to each other, which is why it has gone unnoticed: alternating with ordinary titles, every one is caught. Four parents in a row leave two behind. A parent left in the library is picked up as a job of its own. Its episodes are already queued, so nothing is downloaded twice, but the command makes a directory for it and leaves it empty. Replaced in place instead, which the walk cannot outrun.
The test asserted that no directory appeared, which an implementation that emptied the whole library satisfied just as well -- confirmed by mutation. It now lets the fake resolution add the episodes the real one adds, records which items reach a download, and asserts the exact set: both books and all three episodes, and none of the shows they came from. Three mutations are caught where one was.
Removing the parent podcasts sat in the download command, where it had to be remembered. It moves into `Library.resolve_podcasts` behind a `remove_parents` argument that defaults to false, so listing and exporting keep showing the shows beside their episodes, which is what they are for, and the download asks for them to go because a parent carries no audio of its own. The skip that started this is fixed in the same move: the list is replaced in place rather than walked while items are taken out of it. The test follows the logic. Two cases now sit on the model, where the removal is -- the shows stay by default, and asking takes out all three neighbours -- and the command test proves it asks, with the real method underneath so a fake cannot answer for it. That was worth doing: the old test faked resolution entirely, so once the removal moved it would have gone on passing while testing nothing.
The two lists resolved podcasts with the same four lines, and adding the switch to one of them would have left the other behind. The body moves to `BaseList._resolve_podcasts`, which takes `remove_parents` and passes the rest through to `get_child_items`. `Library` and `Catalog` keep their own signatures, because only one of them has dates to offer, and Wishlist is untouched: it has no `resolve_podcasts` and its items have no children. Both callers are covered, including that a Library still passes its dates down -- a mutation that dropped them went unnoticed until a test asked.
It returned the coroutine of `resolve_podcasts` rather than awaiting it, so `await library.resolve_podcats()` warned about the spelling, handed back a coroutine object and resolved nothing at all. One word. Two tests were also weaker than they read. The fake children left `_children` unset, which the real call sets, so a show that survived a resolve would have tripped over None before reaching the behaviour the comment describes. And the catalog check counted six entries without saying which six.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--resolve-podcastsreplaces every parent podcast with its episodes and thentakes the parents out of the library. It removed them one at a time from the
very list it was walking:
Library.__iter__yields fromself._data, anddatais that same list, soeach removal moves the rest along underneath the walk and the next item is
stepped over.
When it shows
Only where parents sit next to each other, which is why it has gone unnoticed:
What it costs
A parent left in the library becomes a job of its own. Its episodes are
already queued, so nothing is downloaded twice and no file is lost, but the
command creates a directory named after the podcast and leaves it empty.
The fix
The list is replaced in place, which the walk cannot outrun.
The test builds a library with three parents in a row and fails on the old
code with
a parent podcast was still in the library: ['Second_Show'].