Skip to content

[YouTube] Add comment reply count support #936

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
Merged
TobiGr merged 10 commits into from
Oct 15, 2022
Merged

[YouTube] Add comment reply count support #936

TobiGr merged 10 commits into from
Oct 15, 2022

Conversation

@xz-dev
Copy link
Contributor

@xz-dev xz-dev commented Sep 25, 2022

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

Add comment reply count support, just like youtube.

Fix #935

@xz-dev xz-dev mentioned this pull request Sep 25, 2022
Closed
5 tasks
TobiGr
TobiGr requested changes Sep 26, 2022
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.

Thank you for that addition.

Please implement the tests for YouTube.

The reply count should also be extracted for the other services (e.g. PeerTube).

@TobiGr TobiGr added the enhancement New feature or request label Sep 26, 2022
AudricV
AudricV requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

Additional changes that should be made.

Also, please remove the unneeded JavaDoc changes, which were probably made by your IDE.

@xz-dev
Copy link
Contributor Author

xz-dev commented Sep 26, 2022

Additional changes that should be made.

Also, please remove the unneeded JavaDoc changes, which were probably made by your IDE.

Restored

@xz-dev xz-dev requested review from AudricV and TobiGr and removed request for AudricV and TobiGr September 26, 2022 18:17
TobiGr
TobiGr reviewed Sep 26, 2022
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.

Just some small things from my end

@AudricV AudricV removed their request for review September 26, 2022 19:14
@xz-dev xz-dev requested a review from TobiGr September 26, 2022 19:26
@TobiGr
Copy link
Contributor

TobiGr commented Sep 26, 2022

The reply count should also be extracted for the other services (e.g. PeerTube).

Implementing this for PeerTube should be the same effort as you had with YouTube - less than ten lines. Can you please take care of it?

@xz-dev
Copy link
Contributor Author

xz-dev commented Sep 27, 2022 •
edited
Loading

The reply count should also be extracted for the other services (e.g. PeerTube).

Implementing this for PeerTube should be the same effort as you had with YouTube - less than ten lines. Can you please take care of it?

We even not support get the PeerTube comment reply.
So, I think it's that's another job what we need to do but not in this PR.

@xz-dev xz-dev requested review from AudricV and TobiGr and removed request for AudricV and TobiGr September 28, 2022 03:49
@xz-dev
Copy link
Contributor Author

xz-dev commented Sep 28, 2022

I think the pull request is ready been merge, is there any questions?

@xz-dev xz-dev mentioned this pull request Sep 28, 2022
Open
4 tasks
@FireMasterK FireMasterK mentioned this pull request Sep 29, 2022
Merged
3 tasks
@TobiGr
Copy link
Contributor

TobiGr commented Oct 7, 2022 •
edited
Loading

I added a commit to fix exceptions when comments do not have replies and thus no replyCount (197d887)

TobiGr
TobiGr approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@TobiGr TobiGr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you. LGTM now

@AudricV AudricV changed the title Add comment reply count support of Youtube [YouTube] Add comment reply count support Oct 7, 2022
@AudricV AudricV added the youtube service, https://www.youtube.com/ label Oct 7, 2022
@TobiGr TobiGr requested a review from AudricV October 7, 2022 16:41
AudricV
AudricV requested changes Oct 8, 2022
View reviewed changes
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

A few changes remaining and this can be merged in my opinion.

Thank you for this feature!

@TobiGr TobiGr mentioned this pull request Oct 9, 2022
Merged
3 tasks
@xz-dev xz-dev requested a review from AudricV October 14, 2022 02:58
AudricV
AudricV approved these changes Oct 14, 2022
View reviewed changes
Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much!

Please avoid in the future writing usernames with @ in commits, as if they contain these usernames in this form, you would probably send a GitHub notification each time the commits are rebased and/or pushed (that's what happened for me with your latest commits), including in repositories forks if I am not wrong.

This PR will be merged using the Squash and merge command.

@xz-dev
Copy link
Contributor Author

xz-dev commented Oct 15, 2022

Ok, I'll, usernames with @ in commits is github auto add it here, I don't know why github do that, so I keep it here.

@TobiGr TobiGr merged commit 0ffcb32 into TeamNewPipe:dev Oct 15, 2022
@AudricV AudricV mentioned this pull request Oct 29, 2022
Merged
1 task
This was referenced Nov 4, 2022
Merged
Closed
@TobiGr TobiGr mentioned this pull request Dec 3, 2022
Draft
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TobiGr TobiGr TobiGr approved these changes

@AudricV AudricV AudricV approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request youtube service, https://www.youtube.com/

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add comment reply count support

3 participants

@xz-dev @TobiGr @AudricV