xaicron / p5-www-youtube-download

YouTube video download interface.
http://blog.livedoor.jp/xaicron/
Other
38 stars 28 forks source link

Get youtube-playlists working again. (GH#40) #60

Closed isync closed 4 years ago

isync commented 4 years ago

I've restored the ability to download playlists. Simple as that ;)

oalders commented 4 years ago

Thanks for this! I have a few comments. The first thing that jumps out is that this didn't pass the tests:

#   Failed test 'pod coverage for WWW::YouTube::Download'
#   at xt/author/pod-coverage.t line 42.
# Coverage for WWW::YouTube::Download is 80.0%, with 4 naked subroutines:
#   fetch_playlist
#   fetch_playlist_json
#   find_playlist_json_videos
#   find_playlist_videos
# Looks like you failed 1 test of 1.
xt/author/pod-coverage.t ... 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/1 subtests 

...

#   Failed test 'POD test for lib/WWW/YouTube/Download.pm'
#   at /usr/local/lib/perl5/site_perl/5.30.3/Test/Pod.pm line 184.
# lib/WWW/YouTube/Download.pm (861): '=item' outside of any '=over'
# lib/WWW/YouTube/Download.pm (877): You forgot a '=back' before '=head1'
isync commented 4 years ago

Regarding the failed tests, well, the POD formatting errors are now resolved.

But I'm scratching my head about the POD coverage test... Where did you get this one from? (I only see it in github tests) In my repository there's no xt/author/pod-coverage.t - and that's what I worked against. Also, it nags about "internal" methods which I deliberately did not describe. I just didn't prefix them with an underscore as the style of the module doesn't seem to be this way. The described new exposed API method is playlist(),

Could we add some tests for this? My concern is that these changes could break in a subsequent change and we wouldn't know about it. For things that are hard to mock, we could add a live network tests that would run via GitHub Actions.

Mh. It was already a bit of work to get it working again... My selfish idea was to leave this exercise to someone else. Also, I don't know if this module is very prone to being used in automatic systems, a CLI user would simply notice that it stops working if anything changes on YT's end. And the code itself hammers and probes along the way.. and dies on errors.

Also, I don't know anything about github actions. And as the playlist routine munges multiple pages, is sort of reentrant - providing test data, testing every step along the way, etc. is work and in the end might tell us it's working while it's not. You probably noticed it - I'm not up to it... ;)

oalders commented 4 years ago

But I'm scratching my head about the POD coverage test... Where did you get this one from? (I only see it in github tests) In my repository there's no xt/author/pod-coverage.t - and that's what I worked against. Also, it nags about "internal" methods which I deliberately did not describe. I just didn't prefix them with an underscore as the style of the module doesn't seem to be this way. The described new exposed API method is playlist(),

This comes via a Dist::Zilla plugin, so you would see it via dzil test. I would say that if the methods are not meant to be public, let's just prefix them with an underscore and this problem will be solved. :)

Regarding the testing, that's fine. I was just wondering if there's a way we can avoid breaking things in future, but my understanding is that this was already broken for a long time and you've fixed it, so it would be more helpful to get that change in for now. I can maybe add something in GitHub Workflows. I'm not too worried about that right now.

I haven't used this module myself in a long time, but I'm happy to merge and release stuff. Thanks for your time on this. :)

isync commented 4 years ago

Olaf,

ah...! Distzilla. Thanks for the hint. :)

I would say that if the methods are not meant to be public, let's just prefix them with an underscore and this problem will be solved. :)

Done.

..., but my understanding is that this was already broken for a long time and you've fixed it, so it would be more helpful to get that change in for now. I can maybe add something in GitHub Workflows. I'm not too worried about that right now.

Yah, exactly ;) Let's do one step after another. For now, it just works

I haven't used this module myself in a long time, but I'm happy to merge and release stuff. Thanks for your time on this. :)

Yes, it's a little obscure in comparison with youtube-dl, but still, from time to time, it has its place, it is handy and very transparent and available when you need something in PurePerl. I think this back and forth here made the patch a notch better.

Finally, big thank you, Olaf, for maintaining all these modules!!

oalders commented 4 years ago

Finally, big thank you, Olaf, for maintaining all these modules!!

You're very welcome. :) Thanks for working with me on this. I squashed your commits and made some formatting changes. The latest release should be on CPAN shortly. I'm going to close this PR, since I pushed the commits from the command line.