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

WIP (but working) Discord implementation #95

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

Conversation

AridTag
Copy link
Contributor

@AridTag AridTag commented Apr 7, 2018

Depends on PR #94
Fixes #14

Status of the discord implementation is as follows:

Feedback and ideas are desired and most welcome!

Discord Library
  • Using Discord.NET as the discord library

  • Discord client doesn't actually disconnect from discord when you tell it to. This is a known bug and doesn't really cause any problems other than potential confusion for 30 seconds or whatever before discord times out the bot and removes it from the server.

Chat Related
  • I'm not seeing an obvious way to see if a particular SocketMessage came from a DM or from a text channel in the guild. The only differentiating thing I can see is if it's in a guild text channel the user is of type SocketGuildUser and if it's a DM they are a SocketGlobalUser which is unfortunately? an internal type.

  • Currently only supports writing messages to one specific channel defined in the configuration

  • Discord supports markdown so for now I am wrapping all sent messages in backticks as hangman wouldn't show you all the asterisks otherwise!

  • Currently whenever the bot logs in to Discord it sends the Hello and welcome! I hope you're enjoying the steam!... message to the text channel it's set to talk to. It really shouldn't.

  • I think IChatClient SendMessage needs to be overhauled and take more than just plain old string

  • I think the command parser is robust enough but I'm not certain.

    • I tested it using !test slightly "more complicated" --argu"m"e'nts=hello "another quoted string?!" It breaks down to {slightly, more complicated, --argu"m"e'nts=hello, another quoted string}
    • Maybe we can get Magnus to try and break it 😄  
       
Misc. Infrastructure
  • There should be a way to link a Discord user with a user from a streaming platform so their currency is available to them in Discord

    • This ties in to being notified of a new subscriber and adjusting roles in Discord 
       
  • With the way CurrencyGenerator is currently implemented it will begin watching the user if they join Discord or Twitch and will remove the user if they leave either of them. I'm not sure you should even be generating currency for users in Discord at all.

    • Leaving in the context of Discord means choosing to leave the guild. Not just going offline 
       
  • There should be a way to hook Discord into a streaming platform so the bot can be notified when there is a new subscriber and adjust their Roles in Discord if necessary

  • I'm not a huge fan of having to do if (discordUser.RoleIds.Any(role => role == settings.DiscordStreamerRoleId)) for each and every role when determining what UserRole to assign a ChatUser

    • I wonder if a custom JsonConverter should be made so we can define a dictionary in the configuration with UserRole as the key and the DiscordRoleId as the value. Only issue with that is it becomes harder to maintain if you ever rename or remove a UserRole

@AridTag
Copy link
Contributor Author

AridTag commented Apr 7, 2018

I think I've taken this as far as I can without collaboration from you @benrick

All basic functionality is implemented so it's at the point where it wants to branch out into all sorts of places in the bot framework to make changes that I don't feel comfortable making decisions about.

@benrick
Copy link
Member

benrick commented Apr 7, 2018

Yep. I'll pull this in and help work on it.

@AridTag
Copy link
Contributor Author

AridTag commented Apr 7, 2018

Awesome

@AridTag
Copy link
Contributor Author

AridTag commented Apr 10, 2018

I went ahead and rebased this branch

@AridTag AridTag force-pushed the AridTag/Discord branch from 071f163 to 0014d42 Compare May 11, 2018 00:59
@KelsonBall
Copy link

KelsonBall commented May 15, 2018

What's the status of this PR? Is there work here I can pick up?
(edit: hi all, I'm 'greendolph' on twitch/discord)

@AridTag
Copy link
Contributor Author

AridTag commented May 15, 2018

The status is all laid out in the wall of text above. There's a lot of questions about how the core bot framework should be changed to accommodate the needs of the Discord side. The biggest "issue" is that the core framework was developed on stream with a focus on working with Twitch.

I haven't actually ran this in a while but it was connecting to discord and able to process commands as it should.

If you want to contribute to this pull request though, you can make a fork off of my fork and send me pull requests and I can get them merged in to this pull request.

I've been waiting for @benrick to either chime in with thoughts on the issues I laid out or to just jump in and start resolving some of them. But Discord just isn't a priority right now

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.

3 participants