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: use a ChannelManager to handle channel instances #33

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

WIP: use a ChannelManager to handle channel instances #33

wants to merge 7 commits into from

Conversation

PBug90
Copy link
Contributor

@PBug90 PBug90 commented Mar 29, 2018

First implementation attempt. Tries to keep the current API as much as possible. Introduces a new ChannelManager that deals with the channels. Also introduces a new Channel object that deals with a single channel.

@PBug90
Copy link
Contributor Author

PBug90 commented Mar 29, 2018

Maybe it makes sense to dedicate the major version to this feature and also include decent logging with customizable log levels.

@PBug90
Copy link
Contributor Author

PBug90 commented Mar 30, 2018

Refactored and used Standard.js style on the tests for now. https://github.com/standard/standard#why-should-i-use-javascript-standard-style

@PBug90
Copy link
Contributor Author

PBug90 commented Apr 8, 2018

Refactored some of the new code and renamed some variables to better reflect what they are doing in the code.

Also renamed all test files to filename.test.js so that the mocha test call can use a regex to collect the test files and new test files can be added without the need to modify package.json

Added more test cases to test the new implementation.

The TwitchBot constructor now takes a channel array as argument and stores it in an instance variable called _channels. This property is only being used to join the inital channels, The .channels array property is now a function that returns a list of strings of the currently managed channels of the ChannelManager.

@kritzware
Copy link
Owner

Nice work! Is there anything else that needs to be done or is it ready to merge? Does this need any documentation, or have any API changes?

Thanks 😄

@PBug90
Copy link
Contributor Author

PBug90 commented Apr 9, 2018

It's still WIP and not yet ready to merge. I assume it is working properly but it definitely needs more test coverage. Btw, can you add a test coverage reporting tool to the CI pipeline?

I was also thinking about some middleware-implementation that allows interception of specific incoming messages, maybe in a similar fashion like express does?

Since the middleware-result is basically a string we could also implement a promise-chain that can deal with async operations and upon resolving results in the message being sent in the channel. Might make sense to open a new issue for this though.

@kritzware
Copy link
Owner

I'll see if I can set that up sometime soon.

I agree with what you say about middleware functionality, and I also have some ideas about this. I'll make a new issue so it can be discussed there.

@kritzware
Copy link
Owner

Middleware ref: #35

@PBug90
Copy link
Contributor Author

PBug90 commented Apr 11, 2018

I also removed the this.channels[0]-fallback in several utility functions to force the user to provide a valid channel. The user is still free to use a malformed channel name like testchannel or #testChannel but the callback will be called with an information message that the membership in a given channel had not been acknowledged by the irc server yet. Confirmed membership is now required in order to successfully post messages.

Noteworthy:

  • channelManager is still only hooked up with the server-acknowledgement messages that are sent in response to a .join or .part function call on a TwichBot instance
  • botInstance.channels() is now a function that simply calls the channelManager.channels() function and returns its result
  • timeout() and ban() functions are still inconsistent compared to .say() because they do not enforce a callback argument
  • Error handling via callback function is inconsistent because it does not use actual Error objects

@PBug90
Copy link
Contributor Author

PBug90 commented Apr 12, 2018

Updated the docs to reflect the new changes properly. Adjusted the .say() callback argument behaviour so that if the callback is called, it is called with an Error object and the corresponding error message.

@PBug90
Copy link
Contributor Author

PBug90 commented May 13, 2019

If you plan to use this approach in V2, it might be more advisable to use a TokenBucket implementation for the rate limiting instead of my manual approach in here. It would result in the same behaviour but reduces overhead by a lot.

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

Successfully merging this pull request may close these issues.

2 participants