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

Add Page class and remove getNextPageUrl() #314

Merged
merged 5 commits into from
Jul 7, 2020
Merged

Add Page class and remove getNextPageUrl() #314

merged 5 commits into from
Jul 7, 2020

Conversation

wb9688
Copy link
Contributor

@wb9688 wb9688 commented Apr 14, 2020 •
edited
Loading

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Things I'll do in this PR:

  • Remove getNextPageUrl() from ListExtractor
  • Make a Page class to support URL, list of IDs (instead of the current workaround for SoundCloud), and cookies (for YouTube Mix)

@opusforlife2
Copy link
Collaborator

and cookies (for YouTube Mix)

And Youtube Music Mix as well, I assume?

@wb9688
Copy link
Contributor Author

wb9688 commented Apr 15, 2020

@opusforlife2: I haven't looked into YouTube Music Mix, but probably, yes. Note that this PR will only make it possible to add support for Mix, not actually implement support for Mix.

@wb9688 wb9688 marked this pull request as ready for review April 15, 2020 12:13
@wb9688 wb9688 marked this pull request as draft April 15, 2020 14:05
@wb9688 wb9688 mentioned this pull request Apr 15, 2020
Merged
5 tasks
@wb9688 wb9688 marked this pull request as ready for review April 15, 2020 14:24
@wb9688 wb9688 added this to the 0.19.4 milestone Apr 19, 2020
@B0pol B0pol added the enhancement New feature or request label Apr 20, 2020
@XiangRongLin
Copy link
Collaborator

How about a method in the Page to transform the cookie map into a string that can be used for the header when sending a request.
In my local branch i added a util method to util\Util.java for the transformation, but having that method directly in Page would help others know to use that.

@wb9688
Copy link
Contributor Author

wb9688 commented May 11, 2020

@XiangRongLin: I think Utils would be a good place for that as that function isn't specific to cookies.

@wb9688 wb9688 requested a review from TobiGr May 11, 2020 17:37
TobiGr
TobiGr reviewed May 21, 2020
View reviewed changes
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Reviewed most of the files. Only tests and a few other extractors are missing. I'll review the rest tomorrow or on the weekend.

...java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeAccountExtractor.java Outdated Show resolved Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/Page.java Show resolved Hide resolved
@Stypox Stypox modified the milestones: 0.19.4, 0.19.5 May 29, 2020
Stypox
Stypox requested changes Jun 12, 2020
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Just some small things. I like the Page structure and how it is used in SoundCloud ;-D

extractor/src/main/java/org/schabi/newpipe/extractor/ListExtractor.java Outdated Show resolved Hide resolved
...ctor/src/main/java/org/schabi/newpipe/extractor/services/peertube/PeertubeParsingHelper.java Show resolved Hide resolved
...java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeChannelExtractor.java Outdated Show resolved Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeSearchExtractor.java Outdated Show resolved Hide resolved
...ava/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeTrendingExtractor.java Show resolved Hide resolved
...a/org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudChartsExtractor.java Outdated Show resolved Hide resolved
...org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudCommentsExtractor.java Show resolved Hide resolved
extractor/src/main/java/org/schabi/newpipe/extractor/utils/Utils.java Show resolved Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java Outdated Show resolved Hide resolved
TobiGr
TobiGr approved these changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@TobiGr TobiGr requested a review from Stypox June 26, 2020 17:06
Stypox
Stypox requested changes Jun 28, 2020
View reviewed changes
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I didn't check that all of the variables you introduced are actually final, I only saw some ;-)
Btw, after these small things, this is ready to be merged

...java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeAccountExtractor.java Show resolved Hide resolved
.../java/org/schabi/newpipe/extractor/services/peertube/extractors/PeertubeStreamExtractor.java Show resolved Hide resolved
.../org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudChannelExtractor.java Outdated Show resolved Hide resolved
.../org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudChannelExtractor.java Outdated Show resolved Hide resolved
.../org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudChannelExtractor.java Outdated Show resolved Hide resolved
.../org/schabi/newpipe/extractor/services/soundcloud/extractors/SoundcloudChannelExtractor.java Outdated Show resolved Hide resolved
.../java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubePlaylistExtractor.java Outdated Show resolved Hide resolved
@wb9688 wb9688 requested review from Stypox and TobiGr July 6, 2020 18:26
TobiGr
TobiGr reviewed Jul 7, 2020
View reviewed changes
Copy link
Contributor

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Minor stuff

...rc/test/java/org/schabi/newpipe/extractor/services/youtube/YoutubeCommentsExtractorTest.java Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
.../test/java/org/schabi/newpipe/extractor/services/peertube/PeertubeCommentsExtractorTest.java Outdated Show resolved Hide resolved
@wb9688 wb9688 requested a review from TobiGr July 7, 2020 18:46
@TobiGr TobiGr merged commit a70cb02 into TeamNewPipe:dev Jul 7, 2020
@TobiGr TobiGr changed the title Next page stuff Add Page class and remove getNextPageUrl() Jul 7, 2020
This was referenced Jul 11, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@B0pol B0pol B0pol left review comments

@XiangRongLin XiangRongLin XiangRongLin left review comments

@TobiGr TobiGr TobiGr approved these changes

@Stypox Stypox Awaiting requested review from Stypox

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.19.6
Development

Successfully merging this pull request may close these issues.
6 participants
@wb9688 @opusforlife2 @XiangRongLin @TobiGr @Stypox @B0pol