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.

Sign up for GitHub

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

Jump to bottom

Fix cnbc.py / add CNBCVideoIE extractor #8741

Merged
merged 11 commits into from Feb 23, 2024
Merged

Fix cnbc.py / add CNBCVideoIE extractor #8741

merged 11 commits into from Feb 23, 2024

Conversation

gonzalezjo
Copy link
Contributor

@gonzalezjo gonzalezjo commented Dec 10, 2023 •
edited by pukkandan

Description of your pull request and other information

Fixes #8378 by rewriting the CNBC video extractor.

  • Removes CNBCIE, which is an extractor for a CNBC video platform that no longer exists.
  • Updates CNBCVideoIE
    • Adds tests
    • Supports author, channel, formats, ID (of course), upload date, URL (of course), title (of course), description, duration, timestamp, and thumbnails.
Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

Fix cnbc.py / add CNBCVideoIE extractor
cc5692e 
Fixes yt-dlp#8378 by rewriting the CNBC video extractor.

- Removes `CNBCIE`, which is an extractor for a CNBC video platform that no longer exists.
- Updates CNBCVideoIE
    - Adds tests
    - Supports author, channel, formats, ID (of course), upload date, URL (of course), title (of course), description, duration, timestamp, and thumbnails.

Squashed commit of the following:

commit c4c6a3e
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 20:12:51 2023 -0500

    Final cleanups

commit 98837e5
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 20:06:01 2023 -0500

    Rebase 'Further cleanups + better extraction'

    Cool.

commit 210a0e5
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 19:48:58 2023 -0500

    new tests

commit 91fcdfb
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 19:41:45 2023 -0500

    added a test

commit 28a0100
Author: ruiminggu <ruimingg@andrew.cmu.edu>
Date:   Sat Dec 9 19:11:34 2023 -0500

    fixed test

commit 37c4b72
Author: ruiminggu <ruimingg@andrew.cmu.edu>
Date:   Sat Dec 9 19:11:17 2023 -0500

    fixed test

commit 6f42a60
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 18:47:17 2023 -0500

    Remove CNBCIE

    Seems to not exist anymore.

commit d9dd742
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 18:44:11 2023 -0500

    Code cleanups

commit 72e9d82
Merge: b272679 b5260a1
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 18:36:30 2023 -0500

    Merge branch 'rachel-cnbc' into cnbc

commit b272679
Merge: 1823eb9 188191a
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 18:35:33 2023 -0500

    Merge branch 'noor-cnbc' into cnbc

commit 188191a
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 18:34:11 2023 -0500

    Simplify noor-code

commit 7c43a9d
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Sat Dec 9 17:37:00 2023 -0500

    Mostly fix tests

    Nice.

commit db204a5
Author: Noor Mostafa <noormostafa@noors-air-3.wifi.local.cmu.edu>
Date:   Sat Dec 9 17:18:57 2023 -0500

    test case for CNBC video

commit b5260a1
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 17:17:04 2023 -0500

    format upload_date

commit 1823eb9
Author: ruiminggu <ruimingg@andrew.cmu.edu>
Date:   Sat Dec 9 15:55:04 2023 -0500

    modify for properly capitalize the channel

commit c164f09
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 15:38:40 2023 -0500

    add the case where author is NA

commit 3bee446
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 15:26:03 2023 -0500

    upload_date, video_status, author

commit 668503b
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 15:25:08 2023 -0500

    original

commit fb8f173
Author: zhijinwuu <zhijinw@andrew.cmu.edu>
Date:   Sat Dec 9 15:09:44 2023 -0500

    upload_date, video_status, and author

commit b57090b
Author: Noor Mostafa <noormostafa@noors-air-3.wifi.local.cmu.edu>
Date:   Fri Dec 8 16:24:36 2023 -0500

    metadata using builtin functions

commit a3b0068
Author: ruiminggu <ruimingg@andrew.cmu.edu>
Date:   Fri Dec 8 14:59:57 2023 -0500

    channel and duration

commit 09ccfd7
Author: Noor Mostafa <noormostafa@noors-air-3.wifi.local.cmu.edu>
Date:   Thu Dec 7 17:16:17 2023 -0500

    Added metadata for thumbnail, timestamp, and description

commit fcb554e
Author: Noor Mostafa <noormostafa@noors-air-3.wifi.local.cmu.edu>
Date:   Thu Dec 7 14:51:10 2023 -0500

    thumbnail metadata

commit 2101192
Author: J. Gonzalez <gonzalezjo@users.noreply.github.comm>
Date:   Thu Dec 7 12:45:36 2023 -0500

    Basic video extraction supported for CNBC.

    Does the absolute barest minimum.
@gonzalezjo
Copy link
Contributor Author

gonzalezjo commented Dec 10, 2023 •
edited

CCing @Noor-5, @zhijinwuu, and @ruiminggu, who all worked on this (we split the work evenly) and agreed to license their contributions under Unlicense. I'd also like to thank @bashonly for answering a question we had about testing. (Thanks!)

@pukkandan pukkandan added the site-bug Issue with a specific website label Dec 10, 2023
Convert double quotes to single quotes, remove a dead variable, and f…
59e5975 
…ix parentheses style.

After seeing feedback on some other pull requests, I noticed that single quotes were preferred. I've accordingly, and preemptively, converted our single quotes to double quotes. I took the opportunity to double check the style guide and make sure everythiyng else was tidied up as well.
@pukkandan
Copy link
Member

Pls avoid force-pushing. If you happen to do so while someone's reviewing, GH just throws away the comments on the old commit. You can use as many commits as needed. We squash before merge

@gonzalezjo
Copy link
Contributor Author

My apologies for that. I'll avoid force pushes in the future. Thanks for the heads up.

@bashonly bashonly self-requested a review December 12, 2023 00:08
seproDev
seproDev requested changes Feb 2, 2024
View reviewed changes
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
@seproDev seproDev added the pending-fixes PR has had changes requested label Feb 2, 2024
@seproDev seproDev added pending-review PR needs a review and removed pending-fixes PR has had changes requested labels Feb 22, 2024
@pukkandan pukkandan removed the pending-review PR needs a review label Feb 23, 2024
bashonly
bashonly reviewed Feb 23, 2024
View reviewed changes
yt_dlp/extractor/cnbc.py Outdated Show resolved Hide resolved
@seproDev @bashonly
Fix _VALID_URL
4735d62 
Co-authored-by: bashonly <88596187+bashonly@users.noreply.github.com>
@seproDev seproDev merged commit 998dffb into yt-dlp:master Feb 23, 2024
6 checks passed
aalsuwaidi pushed a commit to aalsuwaidi/yt-dlp that referenced this pull request Apr 21, 2024
@gonzalezjo @Noor-5
@zhijinwuu @ruiminggu @seproDev @aalsuwaidi
[ie/cnbc] Overhaul extractors (yt-dlp#8741)
886106e 
Closes yt-dlp#5871, Closes yt-dlp#8378
Authored by: gonzalezjo, Noor-5, zhijinwuu, ruiminggu, seproDev

Co-authored-by: Noor Mostafa <93787875+Noor-5@users.noreply.github.com>
Co-authored-by: zhijinwuu <zhijinw@andrew.cmu.edu>
Co-authored-by: ruiminggu <ruimingg@andrew.cmu.edu>
Co-authored-by: sepro <4618135+seproDev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seproDev seproDev seproDev requested changes

@bashonly bashonly bashonly approved these changes

Assignees

@seproDev seproDev

Labels
site-bug Issue with a specific website
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CNBCVideo error
7 participants
@gonzalezjo @pukkandan @seproDev @bashonly @SirElderling @trainman261 @pzhlkj6612