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

Find url in a new way #56

Merged
merged 1 commit into from Jun 15, 2020
Merged

Find url in a new way #56

merged 1 commit into from Jun 15, 2020

Conversation

kiwiroy
Copy link
Contributor

@kiwiroy kiwiroy commented Jun 2, 2020

This fixes #55, although I'm unsure how long this fix will be successful.

There's some ancillary toolchain/linting edits included so that Travis has a green light - hope that's ok.
Build Status

@coveralls
Copy link

coveralls commented Jun 2, 2020

Pull Request Test Coverage Report for Build 119

  • 20 of 24 (83.33%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+18.1%) to 38.095%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/WWW/YouTube/Download.pm 20 24 83.33%
Totals Coverage Status
Change from base Build 114: 18.1%
Covered Lines: 80
Relevant Lines: 210

💛 - Coveralls

Copy link
Collaborator

@oalders oalders left a comment

Choose a reason for hiding this comment

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

This looks really good. Thanks for doing this. I've added a few comments.

.travis.yml Outdated Show resolved Hide resolved
: $mime =~ m{video/3gp;} ? '3gp'
: 'flv'
;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would https://metacpan.org/pod/MIME::Type help here? Specifically subType()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return $stype now?

lib/WWW/YouTube/Download.pm Outdated Show resolved Hide resolved
lib/WWW/YouTube/Download.pm Outdated Show resolved Hide resolved
t/youtube.t Outdated
check_video_fetch_url('Y1I1KcKvz9Q');

# YAPC video 2
# YAPC video 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing the line endings here. Do any of the examples in this test trigger _is_new()? It would be helpful to cover both cases. This test will only be run if P5_YOUTUBE_NETWORK_TESTS, so we don't have to worry about the average test case for the end user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t/data/player_response.html is trimmed from curl -o ... the first YAPC video. Getting the old content as a test might be a challenge.

@kiwiroy kiwiroy force-pushed the new-find-urls branch 2 times, most recently from 227bb87 to ef68dbb Compare June 4, 2020 11:36
@kiwiroy kiwiroy requested a review from oalders June 4, 2020 11:51
Copy link
Collaborator

@oalders oalders left a comment

Choose a reason for hiding this comment

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

This is failing on 5.10 because it can't find Mock::Quick for some reason. This might be a good time for me to move the repo to GitHub Actions. I can test it out there.

return 1 unless ($args->{fmt_list} and $args->{url_encoded_fmt_stream_map});
return 0;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose we could add a very simple unit test for this as well, to ensure that it is covered.

@@ -578,6 +622,8 @@ Parses given URL and returns playlist ID.

Parses given URL and returns YouTube username.

=item B<get_video_id($video_id)>
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@oalders
Copy link
Collaborator

oalders commented Jun 11, 2020

I just switched to GitHub workflows. Could you delete .travis.yml from this branch and rebase as well?

also pass CI
additional test of new path
@kiwiroy
Copy link
Contributor Author

kiwiroy commented Jun 13, 2020

Rebased and all green.

Copy link
Collaborator

@oalders oalders left a comment

Choose a reason for hiding this comment

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

Thanks @kiwiroy!

@oalders oalders merged commit 2d88545 into xaicron:master Jun 15, 2020
@kiwiroy kiwiroy deleted the new-find-urls branch June 15, 2020 23:37
@oalders
Copy link
Collaborator

oalders commented Jul 31, 2020

This is now on CPAN.

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.

failed to find video urls
3 participants