Migrate /chatters commands to use Helix api #4088

cbclemmer · 2022-10-29T14:36:15Z

Pull request checklist:

CHANGELOG.md was updated, if applicable

Description

closes #4019

Features:

Migrates /chatters command will use helix api or cache
- If the user is neither mod or broadcaster the list is taken from the user list cache
- If the user has mod rights the list is taken from the chat/chatters helix api command
The old irc commands for /chatters have been removed based on comment in issue

github-actions

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 36. Check the log or trigger a new build to see more.

src/common/ChatterSet.cpp

src/controllers/commands/CommandController.cpp

src/providers/twitch/TwitchChannel.cpp

src/providers/twitch/api/Helix.cpp

pajlada

Thanks for working on this!

These changes are both pretty heavy, and while they rely on the same underlying functionality (the cursor-pages-fetching thing), it would be a lot easier to review if I could focus on a single functionality rather than two, since most feedback I have would apply to both.

So in short, could you undo the changes either to the /mods command or to the /chatters command (I have no preference) in this PR?

After that's done, I will go ahead and review the code and come with more proper/full feedback

EDIT: btw, feel free to ignore clang-tidy for now, it can come with some weird suggestions :)

cbclemmer · 2022-10-30T13:22:27Z

@pajlada I totally understand. The only reason I did it this way was because I needed both /mods and /chatters to make the the viewer list be entirely moved to the Helix api. I'll just separate this into /mods and /chatters prs and then once both are merged I'll add a pr that migrates the viewer list.

cbclemmer · 2022-10-30T13:45:19Z

@pajlada Code and pr message has been updated. I've got everything in another branch to merge in later. There were a few functions that will be shared for /chatters and /mods so some functions look like they shouldn't be their own thing, but they'll be useful later.

src/providers/twitch/TwitchChannel.cpp

src/controllers/commands/CommandController.cpp

src/providers/twitch/api/Helix.hpp

src/providers/twitch/api/Helix.cpp

src/providers/twitch/TwitchChannel.cpp

cbclemmer · 2022-10-31T22:30:37Z

@pajlada I changed the helper function for helix to be a more generalized version of makeRequest that can be paginated or not. I'll use this for the /mods pr too. It now can pass in a function that does what it needs to the response from the api and can recursively loop until the end of the list. The /chatters command now just gets one page of chat/chatters api and returns the the total property value. I also changed the error type to be a general error because there isn't any special error types with these requests.

src/providers/twitch/api/Helix.hpp

I did this by running the ./tools/clang-format-all.sh script

The number in the parenthesis should link to the PR that implements the change, which is 4088 in this case

This makes the code readable, and if you need to know the exact type your IDE can hint it at you (or you can check the getChatters function definition)

This makes the diff a teeny tiny bit easier to go through

src/controllers/commands/CommandController.cpp

github-actions

clang-tidy made some suggestions

src/controllers/commands/CommandController.cpp

src/providers/twitch/api/Helix.cpp

src/providers/twitch/api/Helix.hpp

github-actions

clang-tidy made some suggestions

src/controllers/commands/CommandController.cpp

Instead of having a shared method to handle the pagination logic, this adds a private `fetchChatters` method that expects the caller to provide all parameters that would be necessary to make the request. This method can then be used from the public-facing `getChatters` method in a recursive manner, while also keeping the error handling code right in that function. With this change, the success result type has been changed to the `HelixChatters` type (the same used in the private method). By doing this, we can provide a consistent & full API for any users of the `getChatters` API and not have to provide a separate API for the `/chatters` function. Other important changes that came with this was also avoiding the use of raw pointers, the previous code had difficult-to-track pointers & memory leaks for every call that was made (chatterList and page). One potential unbenchmarked downside to the refactored implementation is that we make more sets of users instead of adding them straight onto the same one. This is a concession that is worth it in my opinion as it makes the implementation easier to read. If the code was used in some hot spot we can benchmark and reconsider this. Another bonus change added to this feature is that we fetch 1000 chatters per page using the `first` request query parameter, making us use a lot fewer API requests. This means we'll run less functions recursively and worry less about crashing our stack.

pajlada · 2022-11-01T19:49:59Z

@cbclemmer uno reverse card - I committed some stuff to your PR that I'd like you to review/approve. Most commits are small and do very little, they'd be easy to go through and it's easy to understand why. However, I made a big refactor PR for the pagination logic to change the pattern we use. This changes the pattern for paginated requests in this PR and for future paginated requests to need two functions; 1 public one that users (e.g. CommandController or TwitchChannel) can use which has a simple, single-use API and 1 private one that the public function will use & recursively call.
The big refactor commit can be found here a3aed80 (#4088) and has a commit message where I try to explain all of the changes I made and why.

Let me know if you have any questions!

github-actions

clang-tidy made some suggestions

src/providers/twitch/api/Helix.cpp

tests/src/HighlightController.cpp

cbclemmer · 2022-11-01T21:00:53Z

@pajlada That all looks good. The reason I wanted to try to make a more general request was because the handling for the status codes and resulting errors were largely copied and pasted so I tried to make a framework to reduce that for the stuff I was writing. It does make a lot more sense to refactor everything later when everything is using a standardized way of handling errors like you commented before. I'm mainly a C# dev and garbage collection is usually an afterthought for me because of that, so thanks for ironing that out. Looks good to me 👍

Felanbird

I'm not 100% on the fact that we are deprecating the command very early, but /chatters usage is probably more common among stable users, so we could take another look at this around Feb when the deprecation of all other commands kicks in.

cbclemmer added 17 commits October 9, 2022 14:42

Migrate /chatters to helix api

ade2972

Text clean up

b230adc

Only refresh chatters for mods

8ecb341

Update from master

efe0414

Recursively call next page until end of list or page 50

abe1c6e

Use helix api for viewer list and /chatters command

c7a7e2a

Add todo note

bd080b1

Fetch chatter count from cache when user does not have mod rights

bca041d

Add list of vips to viewer list

90dacda

Moderator list WIP

6ba0a2e

Fix compilation error

59b2e38

Override /mod command

b1c8a0d

format error depending upon user type

5a82905

Update from master

af55ed5

Add warning for extra args in /chatters

e934078

Clean up

6c28042

Update change log

61707aa

This was referenced Oct 29, 2022

Migrate the Viewer List and the /chatters command to Helix API #4019

Closed

Migrate /mods command to Helix API #3971

Closed

github-actions bot reviewed Oct 30, 2022

View reviewed changes

pajlada reviewed Oct 30, 2022

View reviewed changes

Roll back to only change /chatters

03e503b

cbclemmer changed the title ~~Migrate /mods and /chatters commands to use helix api~~ Migrate /chatters commands to use helix api Oct 30, 2022

pajlada requested changes Oct 30, 2022

View reviewed changes

cbclemmer added 2 commits October 30, 2022 11:52

Periodically refresh chatter list for mods

fc268df

Move refreshChatters back to private function

cb74b28

pajlada reviewed Oct 30, 2022

View reviewed changes

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved

pajlada reviewed Oct 30, 2022

View reviewed changes

src/providers/twitch/TwitchChannel.cpp Outdated Show resolved Hide resolved

cbclemmer added 2 commits October 31, 2022 17:21

Fix refresh chatters

9085b29

Remove unused get num chatters

7e16257

Clean up

e271460

pajlada reviewed Oct 31, 2022

View reviewed changes

src/providers/twitch/api/Helix.hpp Outdated Show resolved Hide resolved

cbclemmer and others added 7 commits November 1, 2022 09:46

Move general error to get chatters error

c0618c8

Reformat code

2cbde99

I did this by running the ./tools/clang-format-all.sh script

Merge remote-tracking branch 'origin/master' into cc/Chatterino#4019

559da26

Fix changelog entry

f6cce29

The number in the parenthesis should link to the PR that implements the change, which is 4088 in this case

Use auto for the chatterList in the success lambda

58c18fe

This makes the code readable, and if you need to know the exact type your IDE can hint it at you (or you can check the getChatters function definition)

Undo some old unneccesary changes

7e03bdc

This makes the diff a teeny tiny bit easier to go through

Remove unused variable paginationCursor from Helix::getChatters

817187e

Felanbird requested changes Nov 1, 2022

View reviewed changes

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved

pajlada added 2 commits November 1, 2022 19:20

No longer care what arguments are provided for /chatters

d2069c4

Don't early out for mod rights check - rely on the API error

9d44cb2

github-actions bot reviewed Nov 1, 2022

View reviewed changes

src/controllers/commands/CommandController.cpp Show resolved Hide resolved

pajlada added 2 commits November 1, 2022 20:35

Add mock test function

0f61ea8

github-actions bot reviewed Nov 1, 2022

View reviewed changes

pajlada approved these changes Nov 1, 2022

View reviewed changes

Merge branch 'master' into cc/Chatterino#4019

4e868a0

Felanbird approved these changes Nov 1, 2022

View reviewed changes

Add include to make std::unordered_set<QString> work for Qt5.12

8c710be

pajlada changed the title ~~Migrate /chatters commands to use helix api~~ Migrate /chatters commands to use Helix api Nov 1, 2022

pajlada enabled auto-merge (squash) November 1, 2022 21:20

pajlada merged commit 495f3ed into Chatterino:master Nov 1, 2022

cbclemmer mentioned this pull request Nov 3, 2022

Migrate /mods command to helix API #4103

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate /chatters commands to use Helix api #4088

Migrate /chatters commands to use Helix api #4088

cbclemmer commented Oct 29, 2022 •

edited

Loading

github-actions bot left a comment

pajlada left a comment •

edited

Loading

cbclemmer commented Oct 30, 2022

cbclemmer commented Oct 30, 2022

cbclemmer commented Oct 31, 2022

github-actions bot left a comment

github-actions bot left a comment

pajlada commented Nov 1, 2022

github-actions bot left a comment

cbclemmer commented Nov 1, 2022

Felanbird left a comment

Migrate /chatters commands to use Helix api #4088

Migrate /chatters commands to use Helix api #4088

Conversation

cbclemmer commented Oct 29, 2022 • edited Loading

Description

Features:

github-actions bot left a comment

Choose a reason for hiding this comment

pajlada left a comment • edited Loading

Choose a reason for hiding this comment

cbclemmer commented Oct 30, 2022

cbclemmer commented Oct 30, 2022

cbclemmer commented Oct 31, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

pajlada commented Nov 1, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

cbclemmer commented Nov 1, 2022

Felanbird left a comment

Choose a reason for hiding this comment

cbclemmer commented Oct 29, 2022 •

edited

Loading

pajlada left a comment •

edited

Loading