Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GetVideosAsync fix #821

Closed
wants to merge 3 commits into from
Closed

GetVideosAsync fix #821

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86b9019
Fix #749: Return video in playlist
hunterlan Sep 30, 2024
2bd149b
Code clean up
hunterlan Sep 30, 2024
fa22b13
Add suggestion
hunterlan Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions YoutubeExplode.Tests/PlaylistSpecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a better description?

Suggested change
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 youtube = new YoutubeClient();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var youtube = new YoutubeClient();
// Arrange
var youtube = new YoutubeClient();


// Act
var videos = await youtube.Playlists.GetVideosAsync(PlaylistIds.EnormousDuplicates);

// Assert
videos.Should().HaveCountGreaterOrEqualTo(3_900);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also worth asserting that there are no duplicates

}
}
1 change: 1 addition & 0 deletions YoutubeExplode.Tests/TestData/PlaylistIds.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ internal static class PlaylistIds
public const string UserUploads = "UUTMt7iMWa7jy0fNXIktwyLA";
public const string Weird = "PL601B2E69B03FAB9D";
public const string ContainsLongVideos = "PLkk2FsMngwGi9FNkWIoNZlfqglcldj_Zs";
public const string EnormousDuplicates = "PLI_eFW8NAFzYAXZ5DrU6E6mQ_XfhaLBUX";
}
7 changes: 5 additions & 2 deletions YoutubeExplode/Playlists/PlaylistClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,12 @@ public async IAsyncEnumerable<Batch<PlaylistVideo>> GetVideoBatchesAsync(
);

var videos = new List<PlaylistVideo>();
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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused what fundamentally changed between the old and the current implementation. Can you explain in more detail?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I realised that duplicates might be in the beginning and then my suggestion doesn't cover it, so I will improve solution.

{
var videoId =
videoData.Id
Expand All @@ -105,7 +109,6 @@ public async IAsyncEnumerable<Batch<PlaylistVideo>> GetVideoBatchesAsync(
videoData.Index
?? throw new YoutubeExplodeException("Failed to extract the video index.");

// Don't yield the same video twice
if (!encounteredIds.Add(videoId))
continue;

Expand Down