-
-
Notifications
You must be signed in to change notification settings - Fork 412
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
Add Twitch Connector #1497
Add Twitch Connector #1497
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1497 +/- ##
========================================
Coverage 99.88% 99.89%
========================================
Files 54 56 +2
Lines 3395 3686 +291
========================================
+ Hits 3391 3682 +291
Misses 4 4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice! Especially the docstrings. I've made a bunch of small comments.
I'm keen to think about testing this a little differently. Generally in the past we've always done small unit tests which test each function individually and mock everything else completely. This is very rigid and has caused a right headache when refactoring or changing things later.
I propose we move to a different model where our tests start up a dummy websocket server, mock out the API URL constants to point to our local server and run things end to end through opsdroid with some test skills.
I'm about to do this myself in the GitHub connector refactor, so perhaps you want to hold off writing tests until I've done mine so you have an example to work from?
|
Hello Jacob I've resolved all of the comments that you've made and did some testing of the connector on my end on stream as well. Did some more refactoring and decided to add a The I've been thinking about the webhooks. Currently Twitch asks you to set a lease for the subscription to expire. I assume that Twitch will keep sending events even if our oauth expired (but need to test this out on my end). I'm thinking to somehow add a crontab to run a method that will trigger a token refresh. Should this be done in the connector or on a skill? 🤔 |
|
I would do it in the listen. You are looping there right? So put a check in that says something like |
|
Sorry for the late reply, I've refactored the code a bit and we are not really using the loop to listen the websockets since I made the decision to listen for the "is live" event sent through the webhook and then connect to the chat service |
|
I've been livetesting the connector and the bot on stream for the past 3 streams and I'm trying to squash some annoying bugs. Twitch seems to close the websockets more often when I'm live (even if opsdroid is open on a digital ocean droplet). I tried to do the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are looking at this from the developer end of the telescope because you've been writing it, it's only natural. But I recommend you try and take a step back and think about this experience from the user's perspective.
Try not to give them unnecessary detail about how the connector works and instead tell them about how they can use it.
Also try and include some small skill examples if you can.
|
Very good points haha will change things around and add some examples 😄 |
|
Btw after using digital ocean for a few days I've noticed that for some reason ubuntu keeps killing opsdroid, so I'm afraid websockets is giving us a memory leak of some sort. Not sure how to fix now but might give it a go and try aiohttp websockets support, maybe its a library things - I think slack was using the same websockets lib? 🤔 |
|
Yeah the old slack connector had a memory leak. It seemed to happen when the socket was closed from the remote end and a reconnect happened. Not sure why. We ended up switching to the official Slack python library when they added asyncio support and we haven't seen the problem since. So might be worth looking at how they implemented the websockets support. Looks like they use aiohttp |
|
That's what I thought as well, since it seems to be happening everytime the websocket gets closed by Twitch, so I thought it might be an issue with how I'm trying to reconnect or it's a websocket problem. Thank you for pointing the slack library so I can see how they implement the reconnect will play around and try to solve the issue 👍 |
|
Apologies for all of these pushes, I've tested things out and the connector seems to work okay. Still going to test the websocket connect tomorrow on stream to see if I fixed all the issues. It will probably take me a while to write the tests for it and update the docs but I'm happy with the code so far. Also still need to test the renewal logic on a long session of opsdroid. |
… we disconnect websockets
|
Well I thought the issue with the docs(docstring) was because I split up that line, but that wasn't it. Can't really understand what this means. I thought it was a line that was missing punctuation but doesn't seem like it 🤔 Edit: Yeah... it wasn't the command, so I have no idea haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why things are failing here.
I've made a couple of suggestions just to see if they fix it. But I'm wildly stabbing in the dark.
One thing that comes to mind is that code blocks in rst use double backticks and most of your docstrings use single backticks. However it doesn't seem to be complaining in other places. You may just want to fix it anyway.
In rst you highlight ``code`` like this.
Co-authored-by: Jacob Tomlinson <jacobtomlinson@users.noreply.github.com>
|
Got to say, adding two backticks and a space its a very annoying thing haha but it's fine 😛 I just need to get used to that, I've added two backticks on all of them and also added the space, lets see if that will fix it. |
|
in rst single backtick is a (or can be) a cross-reference. |
|
Ah okay makes sense, and know that I know why it was complaining it makes sense why it was saying start-end issues. Good to see that the thing passes now haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really awesome!! 🚀
I have just reviewed the event handling stuff.
|
So glad to see this go in. It has been a lot of work. Many thanks @FabioRosado. |
Description
This is the initial commit for the Twitch connector, there is still a lot to be done like writing the tests and create the documentation for it. I'm raising this draft PR to get some feedback on the word done and if I can improve things a bit more.
This connector is pretty big because I'm using a lot of things. Twitch has different ways to do stuff so we are using webhooks to get 3 events, then calling the API to update channel titles and websockets to listen to the chat.
I thought about what other bots do and what could be cool to include and that's the reason this connector grew so much haha
Things still to be done
skill-twitchto handle different events and actions_()hub.secretcheck when handling webhook response - prevent folks from sending data that wasn't sent by Twitch.Notes
tokenexpires after 24 hours so we will refresh token when it expires and will re-subscribe to webhookstwitch.json- saved data on appdirdatato keep the oauth token and refresh token saved so we can get/setEvery time we send something through websocket the command is very similar, should we just create a helper method to send messages to the websocket? Will this improve readability?
Status
READY| UNDER DEVELOPMENT |ON HOLDType of change
How Has This Been Tested?
Checklist: