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

Migrate /announce command to Helix API. #4003

Merged
merged 14 commits into from
Sep 24, 2022
Merged

Migrate /announce command to Helix API. #4003

merged 14 commits into from
Sep 24, 2022

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Sep 19, 2022 •
edited by pajlada
Loading

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

github-actions[bot]
github-actions bot reviewed Sep 19, 2022
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Outdated Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
tests/src/HighlightController.cpp Outdated Show resolved Hide resolved
@Wissididom
Copy link
Contributor

Idk if it is important, but shouldn't Helix::sendChatAnnoucement rather be Helix::sendChatAnnouncement (missing n in Announcement)

@Mm2PL Mm2PL changed the title Add Helix announce endpoint Use helix for /announce Sep 24, 2022
@Mm2PL Mm2PL marked this pull request as ready for review September 24, 2022 12:32
@Felanbird
Copy link
Collaborator

Felanbird commented Sep 24, 2022 •
edited
Loading

Can we implement colored announcements in this PR (maybe just the commands)? I believe the only real disdain against them was the fact that IRC invoked announcements did not return color data, which is "resolved" by their deprecation.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 24, 2022

Idk if it is important, but shouldn't Helix::sendChatAnnoucement rather be Helix::sendChatAnnouncement (missing n in Announcement)

fixed

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 24, 2022

Can we implement colored announcements in this PR (maybe just the commands)? I believe the only real disdain against them was the fact that IRC invoked announcements did not return color data, which is "resolved" by their deprecation.

I decided to introduce internal support for sending colored announcements as it was simply more effort to not do it. I didn't want to do commands because I wanted the PR to only move the existing command to helix, just fix the 'bug' and not introduce any new features at the same time. :)

@Mm2PL
Merge branch 'master' of github.com:Chatterino/chatterino2 into fix/f…
6463b0f 
…uck_you_twitch_this_api_is_so_fucking_shit_stop_breaking_our_program_AAAAAAAAA

I hate merge conflicts
@Mm2PL Mm2PL requested a review from pajlada September 24, 2022 13:04
@Mm2PL Mm2PL changed the title Use helix for /announce Minor: Migrate /announce command to Helix API. Sep 24, 2022
@Mm2PL Mm2PL changed the title Minor: Migrate /announce command to Helix API. Migrate /announce command to Helix API. Sep 24, 2022
github-actions[bot]
github-actions bot reviewed Sep 24, 2022
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/providers/twitch/api/Helix.cpp Outdated Show resolved Hide resolved
tests/src/HighlightController.cpp Outdated Show resolved Hide resolved
@pajlada
Reorder code
ee4fecc
@pajlada
Reorder changelog entry
b0d3b5e
@pajlada
clang-tidy: *
230662b
@pajlada
auto in failure callback
35146e8
@pajlada
misc changes
00ecd73 
Formatting consistency

`Error` instead of `ErrorT` for the error type
Rename error type from `HelixAnnouncementError` to `HelixSendChatAnnouncementError`
Remove unused error type (UserNotAuthorized)
Remove unused error handling code
Remove period from forwarded messages as the messages seem mostly to include periods.
Remove c-v-pasted comment
Correct test mock failure return type
@pajlada
Add error code 400 handler
132d24a
@pajlada
add comment for future pajlada
d90f40e
@pajlada
Merge remote-tracking branch 'origin/master' into fix/fuck_you_twitch…
8c5207b 
…_this_api_is_so_fucking_shit_stop_breaking_our_program_AAAAAAAAA
pajlada
pajlada approved these changes Sep 24, 2022
View reviewed changes
Copy link
Member

@pajlada pajlada 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!
Letting you hit the Merge button so you have an opportunity to complain about the changes I've made @Mm2PL

github-actions[bot]
github-actions bot reviewed Sep 24, 2022
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
src/providers/twitch/api/Helix.cpp Show resolved Hide resolved
tests/src/HighlightController.cpp Show resolved Hide resolved
@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 24, 2022

ibana.pjg

@Mm2PL Mm2PL merged commit 8bda8a8 into master Sep 24, 2022
@Mm2PL Mm2PL deleted the fix/fuck_you_twitch_this_api_is_so_fucking_shit_stop_breaking_our_program_AAAAAAAAA branch September 24, 2022 15:50
@Felanbird Felanbird mentioned this pull request Sep 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@pajlada pajlada pajlada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Mm2PL @Wissididom @Felanbird @pajlada