Skip to content

Remove the need for a CommentsInfo instance in CommentsInfo.getMoreItems and fix PeertubeCommentsExtractorTest.Default test#807

Merged
litetex merged 1 commit intoTeamNewPipe:devfrom
FireMasterK:no-commentsinfo-instance
Mar 15, 2022
Merged

Remove the need for a CommentsInfo instance in CommentsInfo.getMoreItems and fix PeertubeCommentsExtractorTest.Default test#807
litetex merged 1 commit intoTeamNewPipe:devfrom
FireMasterK:no-commentsinfo-instance

Conversation

@FireMasterK
Copy link
Copy Markdown
Member

@FireMasterK FireMasterK commented Mar 3, 2022 •
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.

I also fixed the Peertube comments test.

Closes #697, this is important for Piped since these instances are currently cached in memory

@FireMasterK FireMasterK force-pushed the no-commentsinfo-instance branch from a25cb84 to 5e06788 Compare March 3, 2022 11:45
@FireMasterK FireMasterK force-pushed the no-commentsinfo-instance branch from 5e06788 to 60cc71e Compare March 3, 2022 11:48
@FireMasterK FireMasterK mentioned this pull request Mar 3, 2022
Merged
@AudricV AudricV changed the title Remove the need for a CommentsInfo instance Remove the need for a CommentsInfo instance in CommentsInfo.getMoreItems and fix PeertubeCommentsExtractorTest.Default test Mar 12, 2022
@AudricV AudricV added enhancement New feature or request PeerTube Service, https://joinpeertube.org/ multiservice Issues related to multiple services labels Mar 12, 2022
litetex
litetex approved these changes Mar 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM

commentsInfo.getCommentsExtractor().fetchPage(); should be tested with NewPipe

@FireMasterK
Copy link
Copy Markdown
Member Author

FireMasterK commented Mar 15, 2022

commentsInfo.getCommentsExtractor().fetchPage(); should be tested with NewPipe

I have built and tested this with NewPipe, and it worked perfectly. Are you referring to Unit tests? I think they can be added in a different PR

@litetex
Copy link
Copy Markdown
Member

litetex commented Mar 15, 2022

I have built and tested this with NewPipe, and it worked perfectly.

Ok, that's great. Then merge it is 😄

Are you referring to Unit tests? I think they can be added in a different PR

No (there is already #813 for fixing these problems)

@litetex litetex merged commit e7aee0c into TeamNewPipe:dev Mar 15, 2022
@FireMasterK FireMasterK deleted the no-commentsinfo-instance branch March 15, 2022 18:12
@litetex litetex mentioned this pull request Mar 26, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Stypox Stypox Stypox approved these changes

@litetex litetex litetex approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request multiservice Issues related to multiple services PeerTube Service, https://joinpeertube.org/

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CommentsInfo.getMoreItems requires a CommentsInfo instance

4 participants

@FireMasterK @litetex @Stypox @AudricV