GetVideosAsync fix #821
GetVideosAsync fix #821
Conversation
Added additional variable, which will not contain videos, that were already added.
| [Fact] | ||
| public async Task I_can_get_videos_included_in_a_buggy_playlist() | ||
| { | ||
| var youtube = new YoutubeClient(); |
There was a problem hiding this comment.
| var youtube = new YoutubeClient(); | |
| // Arrange | |
| var youtube = new YoutubeClient(); |
| @@ -189,4 +189,16 @@ public async Task I_can_get_a_subset_of_videos_included_in_a_playlist() | |||
| // Assert | |||
| videos.Should().HaveCount(10); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task I_can_get_videos_included_in_a_buggy_playlist() | |||
There was a problem hiding this comment.
Is it a better description?
| public async Task I_can_get_videos_included_in_a_buggy_playlist() | |
| public async Task I_can_get_videos_included_in_a_playlist_with_a_lot_of_duplicates() |
| var videos = await youtube.Playlists.GetVideosAsync(PlaylistIds.EnormousDuplicates); | ||
|
|
||
| // Assert | ||
| videos.Should().HaveCountGreaterOrEqualTo(3_900); |
There was a problem hiding this comment.
Maybe also worth asserting that there are no duplicates
| var originalVideos = response.Videos.Where(v => | ||
| { | ||
| return !encounteredIds.Any(e => string.Equals(e.Value, v.Id)); | ||
| }); | ||
|
|
||
| foreach (var videoData in response.Videos) | ||
| foreach (var videoData in originalVideos) |
There was a problem hiding this comment.
I'm confused what fundamentally changed between the old and the current implementation. Can you explain in more detail?
There was a problem hiding this comment.
I'm confused what fundamentally changed between the old and the current implementation. Can you explain in more detail?
Sorry for late answer. To be honest it was hard for me to detect it (and even harder to explain it), but I noticed that sometimes our duplicates can be as a last list element and while they are not added to encounteredId, they are still used in JSON key playlistIndex. That's why when we are reaching value of 2039 in encounteredId, we have value 811 of lastVideoIndex. So, to be sure that we don't work with duplicates, we can remove them before foreach.
It's probably make sense to remove if condition in foreach and just add videoId to encounteredId, since we removing duplicates.
There was a problem hiding this comment.
BTW, I realised that duplicates might be in the beginning and then my suggestion doesn't cover it, so I will improve solution.
Added all suggested changes from PR and improved duplicate filtration.
| IEnumerable<PlaylistVideoData> originalVideos; | ||
| if (!encounteredIds.Any()) | ||
| { | ||
| originalVideos = response.Videos.DistinctBy(v => v.Id); | ||
| } | ||
| else | ||
| { | ||
| originalVideos = response.Videos.Where(v => | ||
| { | ||
| return !encounteredIds.Any(e => string.Equals(e.Value, v.Id)); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I still think the old code and the new code work the same 🤔
Previously, we used a HashSet to deduplicate video IDs. Using the following block:
if (!encounteredIds.Add(videoId))
continue; // this will execute if the `videoId` already existsAnd now we use this instead:
if (!encounteredIds.Any())
{
originalVideos = response.Videos.DistinctBy(v => v.Id);
}
else
{
originalVideos = response.Videos.Where(v =>
{
return !encounteredIds.Any(e => string.Equals(e.Value, v.Id));
});
}Which seems to do the same.
There was a problem hiding this comment.
You can run it by yourself and see that my changes allow to download more videos. I can't explain by words, why this changes works better than code on master tree. My observation just showed that for some reasons, on some point (probably when last item is duplicate), it starts to return videos, that were already added to HashSet.
Closes #749
For some reasons, YouTube provided videos, which were already added and this caused to stop function to continue fetch next videos.
Added a separate list, which filter
response.Videosby searching added video ID inencounteredIds.