-
Notifications
You must be signed in to change notification settings - Fork 208
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.
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
Fix duration parsing for very long videos #661
Conversation
So I'm not sure what the correct behavior is here... Afaik from sources on the internet, YT's max video duration is 12h. Since the video you linked is not playable either I would assume that it's simply an invalid entry. Not sure we'd even want to parse the duration in that case. Granted we should not crash but return |
The fix you provided here is also locale dependent and would break for German localization
|
Good point, I did not think of this. If the locale is known then we can use the function But if this is not possible then this the way:
|
Are there any videos with 1k+ hours that are actually playable? If yes I'd have a go with atoi. If no, don't bother and just return None |
fyi there is already a ytmusicapi/ytmusicapi/helpers.py Line 60 in 2b5d19a
|
I think they are not playable. The current longest playable video on youtube I was able to find is about 300hrs (https://youtu.be/kj4LxVhx2-A). So just returning None should be fine. |
@abhishekmj303 ok yes, let's do that. I guess you could check if the result of |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 94.99% 95.09% +0.09%
==========================================
Files 38 38
Lines 2278 2282 +4
==========================================
+ Hits 2164 2170 +6
+ Misses 114 112 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@sigma67 Not sure what tests and where I need to add them. Can you help me here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test with a playlist that contains an applicable song with >1k hours
see https://github.com/sigma67/ytmusicapi/blob/main/tests/README.rst
@abhishekmj303 Thanks! Everything looks good now |
Fixes #660