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

[Instagram] Add new [:tag] extractor, refactor [:user] #18757

Merged
merged 3 commits into from
Jan 20, 2019
Merged

[Instagram] Add new [:tag] extractor, refactor [:user] #18757

merged 3 commits into from
Jan 20, 2019

Conversation

jhwgh1968
Copy link
Contributor

@jhwgh1968 jhwgh1968 commented Jan 6, 2019

Before submitting a pull request make sure you have:

In order to be accepted and merged into youtube-dl each piece of code must be in public domain or released under Unlicense. Check one of the following options:

  • 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?

  • Bug fix
  • Improvement
  • New extractor
  • New feature

Description of your pull request and other information

This adds an extractor for Instagram videos. It downloads all videos that match a given hashtag as a playlist. It is heavily based on the Instagram user extractor.

I did consider refactoring common methods instead of copy-pasting, but decided it was more important to avoid touching the other code. This way is more difficult to maintain, but I hope is easier to review and avoids the risk of regressions in the other extractors.

If you would prefer a refactor -- either now or in a separate PR -- I would be happy to do it.

@dstftw
Copy link
Collaborator

dstftw commented Jan 6, 2019

No copy pasting here.

@jhwgh1968 jhwgh1968 changed the title [Instagram:tag] Add new extractor [Instagram] Add new [:tag] extractor, refactor [:user] Jan 6, 2019
@jhwgh1968
Copy link
Contributor Author

@dstftw, I have refactored the utility functions out as you requested.

youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
@jhwgh1968
Copy link
Contributor Author

I have fixed two of the issues @dstftw, but I need clarification on the third one. Where is the repetition?

@jhwgh1968
Copy link
Contributor Author

I have reworked the classes as you requested, @dstftw. I am pretty sure there is nothing left to repeat now.

youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
youtube_dl/extractor/instagram.py Outdated Show resolved Hide resolved
@jhwgh1968
Copy link
Contributor Author

Sorry for the extra changes, @dstftw. I did not revert all of my first attempt at the code which did not use a superclass before, and I should have.

I have put that block back the way it originally was, as you requested. However, I also added a couple of comments, because it was not obvious to me at first why it had to be a class member.

I think your other comments should be addressed also.

@jhwgh1968
Copy link
Contributor Author

Ping @dstftw, I think I have fixed everything. Is there anything else I need to do?

@dstftw dstftw merged commit 31fbedc into ytdl-org:master Jan 20, 2019
@jhwgh1968 jhwgh1968 deleted the instagram_tag branch January 20, 2019 22:42
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