-
Notifications
You must be signed in to change notification settings - Fork 23
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
Specify listeners in config file #155
base: master
Are you sure you want to change the base?
Conversation
@Christopher-Chianelli, I can't actually request your review for whatever reason, but could I get your review on this as well? |
b186f50
to
5441264
Compare
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 recommend using Classes instead of Strings to make missing classes a compile time error + make renaming classes via refactoring easier.
An alternative approach would be to use Quarkus to run VileBot1, and each service would be @ApplicationScoped
, then you don't need to specify the services in Vilebot.java
.
The third approach is to containerize the services using PodIRCBot
(which is what I will do, one day...).
@@ -79,6 +81,12 @@ | |||
|
|||
private static Map<String, String> cfg = getConfigMap(); | |||
|
|||
private static final String[] allListenerClasses = { "AnswerQuestion", "Ascii", "ChatLogger", "Church", "Countdown", |
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 recommend this being a Class array instead of a String array, so that a missing class is a compile time error and to make it easier to rename classes when refactoring.
In regards to "Dynamically generated !help commands for each bot instance.", each service would need to implement an interface. Since Proposal:
and then build a |
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.
Nice work @Kevin-Mok! Left some minor comments in the PR.
@@ -113,20 +121,73 @@ public static void main( String[] args ) | |||
String ircServerAddress = cfg.get( "ircServerAddress" + i ); | |||
int ircPort = Integer.parseInt( cfg.get( "ircPort" + i ) ); | |||
String ircChannel = cfg.get( "ircChannel" + i ); | |||
String listenerCsvString = cfg.get( "listeners" + i ); |
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 additional cfg entry should also be added in vilebot/cfg/vilebot.conf.example
(ie. listeners1=all
).
} | ||
|
||
botManager.start(); | ||
// Done | ||
} | ||
|
||
/** | ||
* Returns list of Listener object based on a CSV string of Listener class names. |
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.
* Returns list of Listener object based on a CSV string of Listener class names. | |
* Returns list of Listener objects based on a CSV string of Listener class names. |
Desired Behavior
Each bot instance should have specified listeners for their specific purpose.
Current Behavior
Each bot instance includes all listeners.
Additional Info
I noticed that the current 4 bots all are using the same configuration that includes all the listeners, even though 3 of the bots have very niche purposes (games in their specific channel). For example, we don't need say
!fortune
for CountdownB0t1. As such, I thought it would be more efficient and make sense to be able to create different configurations for each bot instance so they only use the listeners they need.Another use case I had in mind for this would be to setup a lightweight bot instance where its only job is to periodically says a command. For example, RepeatB0t1 could say
!weather
every hour in the#weatherTO
channel to prompt WeatherB0t1 to output the weather (bots can't trigger their own commands). WeatherB0t1 would also only need one listener, and I thought it would be overkill to spawn these bots with every single listener.Implementation
I refactored the part of the code that sets up each bot's configs. It now reads the
listenersX
property fromvilebot.conf
, which is a comma-separated list of class name values. An example list would beKarma,Weather
orall
. It currently only adds the all the admin handlers to each instance.I also implemented the fix for this unwieldy line as suggested by @amisevsk, as well as using the
addListeners
method to make each line a much more reasonable length:VileBot/vilebot/src/main/java/com/oldterns/vilebot/Vilebot.java
Lines 120 to 121 in aff8a83
Tests
I setup 2 bot instances with the following config:
And tried the following commands with both of them in the same room:
Next Steps
Dynamically generated
!help
commands for each bot instance.