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

Chat commands api #3209

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Chat commands api #3209

wants to merge 4 commits into from

Conversation

fourtf
Copy link
Member

@fourtf fourtf commented Aug 29, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Adds tab completion for bot commands using an api.

@Mm2PL Mm2PL self-requested a review August 29, 2021 19:16
CHANGELOG.md Outdated Show resolved Hide resolved
@ALazyMeme
Copy link
Collaborator

How does this pr work for commands that a user has disabled?

@Felanbird
Copy link
Collaborator

Felanbird commented Aug 30, 2021

How does this pr work for commands that a user has disabled?

Fossabot does not return information via their API for disabled commands, not sure about streamelements.
edit: Streamelements unfortunately does, but only for custom commands, default commands that are disabled are not returned.

edit#2: Streamelements fixed in Chatterino/command-list-api@895cc91

@ALazyMeme
Copy link
Collaborator

This works

@ALazyMeme
Copy link
Collaborator

I feel like the changelog should be:
- Major: Added support for bot command autocompletion using Chatterino's command list api: https://github.com/Chatterino/command-list-api

Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

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

Functionality works as intended. 👍

On my first attempt testing this PR I was unable to properly replicate it in most channels, but I now realize I would have been the first person generating command lists for said channels, and if I had restarted then, the functionality would have worked.

@Felanbird
Copy link
Collaborator

I feel like the changelog should be:
- Major: Added support for bot command autocompletion using Chatterino's command list api: https://github.com/Chatterino/command-list-api

Yeah this is probably better actually, especially if the future intention is to implement other bots.

@@ -2,6 +2,7 @@

## Unversioned

- Major: Added the ability to autocomplete Fossabot & Streamelements commands. (#3209)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Major: Added the ability to autocomplete Fossabot & Streamelements commands. (#3209)
- Major: Added support for bot command autocompletion using Chatterino's command list API: https://github.com/Chatterino/command-list-api (#3209)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that an average Chatterino user would care about the github link in a changelog entry. If someone's interested more into how this feature is going to work they have a PR number and can read up detail on the PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Major: Added the ability to autocomplete Fossabot & Streamelements commands. (#3209)
- Major: Added support for bot command autocompletion using Chatterino's command list API. (#3209)

@ALazyMeme
Copy link
Collaborator

Fixes #2793

@zneix zneix self-requested a review September 11, 2021 11:01
Copy link
Collaborator

@Mm2PL Mm2PL left a comment

Choose a reason for hiding this comment

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

This is a bit of a necro-review.

I like the feature, however there is a problem: when commands are loaded for the first time for a channel the API used to fail, this PR does not account for that. IMO that behavior should either be changed there or accounted for here.

return Success;
})
.onError([channelId](NetworkResult result) {
qCWarning(chatterinoFfzemotes)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a copy/paste mistake. I wouldn't expect custom chat commands to use FFZ logic.

Suggested change
qCWarning(chatterinoFfzemotes)
qCWarning(chatterinoTwitch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants