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

[SoundCloud] Add support for comment replies #993

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

TobiGr
Copy link
Member

@TobiGr TobiGr commented Dec 3, 2022

Follow up on #936 and #941

To Do:

  • self review
  • finish some comments
  • checkstyle
  • Fix getting all comments & replies: caching problems after getting replies leading to wrong comments when getting the next page for the original comment tree.

@TobiGr TobiGr added enhancement New feature or request soundcloud service, https://soundcloud.com/ labels Dec 3, 2022
@TobiGr TobiGr force-pushed the feat/soundcloud_replies branch 4 times, most recently from 75d875f to 9ae1be7 Compare December 4, 2022 15:54
@TobiGr TobiGr marked this pull request as ready for review December 4, 2022 15:59
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.

Looks mostly good to me, but it looks like at SoundCloud they overcomplicated comment replies 😅

}
}
replyCount = replies.size();
if (collector.getItems().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess you are collecting items here to make sure not to show that there are replies when actually all replies give an error when collected.

  • Since you are already collecting replies here, can't you set the items as the repliesPage content directly instead of passing the json again? Or maybe make a Serializable structure of some sort. To avoid recollecting them again later.
  • Or even better, I would skip this check completely and avoid collecting items beforehand. If one of the comments yields an error while being collected, then the error would be shown in the frontend and it is expected that in case of error on an item the list of replies might be empty.

@TobiGr TobiGr marked this pull request as draft January 2, 2023 20:55
Implemented a cache.
TODO: Do not store in cache when viewing replies....
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request soundcloud service, https://soundcloud.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants