Fix page_size limited at 50 on YouTube#320
Conversation
Contextualist
left a comment
Contextualist
left a comment
There was a problem hiding this comment.
Could be a bit verbose, but overall organization and comments are clear, and here are some suggestions to simplify things.
|
Thanks for the suggestion @Contextualist ! I've applied them, let me know if you see other things that need to be changed 🙂 |
Contextualist
left a comment
Contextualist
left a comment
There was a problem hiding this comment.
Looks good! One minor change, and also make sure that you follow the suggestions from the lint, in this case, camelCase and formatting the files.
There are a couple complains from CI's linter. |
|
Thanks for your feedback guys ! The problems should now be resolved 😎 |
|
I might be a bit pedantic 😉 :
|
|
Good call ! I've applied those changes too 😎 |
Contextualist
left a comment
Contextualist
left a comment
There was a problem hiding this comment.
LGTM! Waiting for opinions from @mxpv
Also attaching a better diff for reference:
268 for _, s := range playlist {
269 ids = append(ids, s.ResourceId.VideoId)
270 }
-272 req, err := yt.client.Videos.List("id,snippet,contentDetails").Id(strings.Join(ids, ",")).Context(ctx).Do(yt.key)
+ 273 // Init a list that will contains the aggregated strings of videos IDs (capped at 50 IDs per API Calls)
+ 274 idsList := make([]string, 0, 1)
+ 276 // Chunk the list of IDs by slices limited to maxYoutubeResults
+ 277 for i := 0; i < len(ids); i += maxYoutubeResults {
+ 278 end := i + maxYoutubeResults
+ 279 if end > len(ids) {
+ 280 end = len(ids)
+ 281 }
+ 282 // Save each slice as comma-delimited string
+ 283 idsList = append(idsList, strings.Join(ids[i:end], ","))
+ 284 }
+ 286 // Show how many API calls will be required
+ 287 log.Debugf("Expected to make %d API calls to get the descriptions for %d episode(s).", len(idsList), len(ids))
+ 289 // Loop in each slices of 50 (or less) IDs and query their description
+ 290 for _, idsI := range idsList {
+ 291 req, err := yt.client.Videos.List("id,snippet,contentDetails").Id(idsI).Context(ctx).Do(yt.key)
292 if err != nil {
293 return errors.Wrap(err, "failed to query video descriptions")
294 }
|
Can't wait for this to merged! Thanks so much for developing it @Th0masL and for reviewing it @Contextualist 👌 |
|
Thanks! |
4 participants
This PR fixes the
page_sizelimited to50episodes on YouTube (see issue #306).This was due to the fact that the function
queryVideoDescriptionswas sending an API call with more than50video IDs, and was therefore crashing.What I'm doing is basically to break down the video IDs by chunks of
50, and loop as many times as requires to query their Description.I've also added a DEBUG message that shows how many API calls are expected to be made based on this number of episodes.
And since I was doing this PR, I've also added a small log line to say when an Episode download is skipped because it has already been downloaded earlier, which is something I was always wondering.
PS: Since I had to add some tabulations to a good portion of the function, the
diffis super ugly, and it looks like I've changed way more stuff than what I actually did 😞