search

norkunas / youtube-dl-php

A PHP wrapper for youtube-dl or yt-dlp.
MIT License
450 stars 155 forks source link

Additional source and playlist collection fix #174

Closed Mrlaminat closed 2 years ago

norkunas commented 2 years ago

Would be nice to add a test that it properly parses output with the pc in it

Mrlaminat commented 2 years ago

Would be nice to add a test that it properly parses output with the pc in it

Need to check how tests are implemented for your system. Is it has some prepared URL?

norkunas commented 2 years ago

Would be nice to add a test that it properly parses output with the pc in it

Need to check how tests are implemented for your system. Is it has some prepared URL?

No, it does not do real requests, only tests how it works with the stream output

Mrlaminat commented 2 years ago

@norkunas Fixed regex.

PS. I would recommend you add a regex container, from which all regexes could be tested. It will help to cover more supported platforms without huge regex complexity. If you worrying about performance, maybe you can add a function flag ("useMultiplePlatformRegex", by default false).

norkunas commented 2 years ago

@Mrlaminat I'm not against it, but currently regexes are simple enough not to introduce your suggested regex container. We'll see in future if it become hard to maintain then it probably will be time for that. Will you be able to write a test for this?

norkunas commented 2 years ago

Also I'm curious which source adds the pc to that sentence? Or is it with the yt-dlp that it adds different source to it?

Mrlaminat commented 2 years ago

@Mrlaminat I'm not against it, but currently regexes are simple enough not to introduce your suggested regex container. We'll see in future if it become hard to maintain then it probably will be time for that. Will you be able to write a test for this?

It was just suggestion for the future, right now is good enough!

I just fixed code to pass already existed tests, from my local env I even can't run them with xDebug so it's difficult to find how to create a new one.

LIke for me it works perfect, and it's not broke usuall youtube flow, so looks like we are okay :)

norkunas commented 2 years ago

just run with XDEBUG_MODE=off :)

norkunas commented 2 years ago

Oh, and why do you need to run with it ? :D Because there are not tests that covers the part with Downloading pc webpage so it's better to have at least 1 test to not break this accidentally in future :)

norkunas commented 2 years ago

Ok then if you'd like for me to add the test, could you run the lib in debug mode and give me all output it prints?

norkunas commented 2 years ago

Why you have closed this?

Mrlaminat commented 2 years ago

@norkunas changed my mind. Sorry for the confusion and thank you

norkunas commented 2 years ago

Just not to bother you, open and I'll merge as is :bow:

norkunas commented 2 years ago

Thank you!

Mrlaminat commented 2 years ago

Man, that's a Great project, thank you for developing and maintaining it!