sigma67 / ytmusicapi

Unofficial API for YouTube Music
https://ytmusicapi.readthedocs.io
MIT License
1.76k stars 209 forks source link

Fix duration parsing for very long videos #661

Closed abhishekmj303 closed 1 month ago

abhishekmj303 commented 1 month ago

Fixes #660

sigma67 commented 1 month ago

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 None as duration instead.

sigma67 commented 1 month ago

The fix you provided here is also locale dependent and would break for German localization

duration: 13.165:23:44

abhishekmj303 commented 1 month ago

The fix you provided here is also locale dependent and would break for German localization

duration: 13.165:23:44

Good point, I did not think of this. If the locale is known then we can use the function locale.atoi. Reference

But if this is not possible then this the way:

Granted we should not crash but return None as duration instead.

sigma67 commented 1 month ago

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

sigma67 commented 1 month ago

fyi there is already a to_int function that uses atoi

https://github.com/sigma67/ytmusicapi/blob/2b5d19abb1820cb719b5abc817370aee90b9c5f5/ytmusicapi/helpers.py#L60

abhishekmj303 commented 1 month ago

Are there any videos with 1k+ hours that are actually playable?

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.

sigma67 commented 1 month ago

@abhishekmj303 ok yes, let's do that.

I guess you could check if the result of duration.split(":") contains any non-digit characters and then fall back to None

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.09%. Comparing base (2b5d19a) to head (faf1dc5). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/sigma67/ytmusicapi/pull/661/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigma67) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/sigma67/ytmusicapi/pull/661/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigma67) | `95.09% <100.00%> (+0.09%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigma67#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

abhishekmj303 commented 1 month ago

@sigma67 Not sure what tests and where I need to add them. Can you help me here?

sigma67 commented 1 month ago

@abhishekmj303 Thanks! Everything looks good now