Skip to content
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

Additional source and playlist collection fix #174

Merged
merged 4 commits into from
Dec 29, 2021

Conversation

Mrlaminat
Copy link
Contributor

No description provided.

@Mrlaminat Mrlaminat mentioned this pull request Dec 28, 2021
Copy link
Owner

@norkunas norkunas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your findings :)

src/YoutubeDl.php Outdated Show resolved Hide resolved
src/YoutubeDl.php Outdated Show resolved Hide resolved
@norkunas
Copy link
Owner

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

Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
@Mrlaminat
Copy link
Contributor Author

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?

Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
@norkunas
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Owner

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
Copy link
Contributor Author

@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
Copy link
Owner

just run with XDEBUG_MODE=off :)

@norkunas
Copy link
Owner

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 :)

@Mrlaminat Mrlaminat closed this Dec 29, 2021
@Mrlaminat Mrlaminat reopened this Dec 29, 2021
@norkunas
Copy link
Owner

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?

@Mrlaminat Mrlaminat closed this Dec 29, 2021
@norkunas
Copy link
Owner

Why you have closed this?

@Mrlaminat
Copy link
Contributor Author

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

@norkunas
Copy link
Owner

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

@Mrlaminat Mrlaminat reopened this Dec 29, 2021
@norkunas norkunas merged commit 51e352c into norkunas:master Dec 29, 2021
@norkunas
Copy link
Owner

Thank you!

@Mrlaminat
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants