Update channel topic when Test Day starts and ends #34
Conversation
| @@ -16,6 +16,9 @@ var ircServer = config.server, | |||
| helpers = config.helpers, | |||
| startTime = Date.now(), | |||
| endTime = startTime, | |||
| topic = "", | |||
| qaTopic = "", | |||
| qaChannel = config.channels[0], | |||
There was a problem hiding this comment.
The bot should update the topic in each of the channels, it has joined. So we don't need a special casing here.
There was a problem hiding this comment.
We need a variable to save the pre-Test Day topic, to restore it after the Test Day.
I have been thinking that the bot runs only in #qa (fennecbot appears to run only in #mobile, though it advertises to several other channels). Is that not correct?
There was a problem hiding this comment.
That's correct. The simplest solution would be to change config.channels to a single channel. If needed we could expand it to an array later.
There was a problem hiding this comment.
You may also want to name the variable which holds the former topic as topic_backup or similar but not with a channel name included.
galgeek
changed the title
|
hi @whimboo ! thanks for your feedback. I've tidied up the commits, but I haven't made any other changes: I have been thinking that the bot runs in qa, though it also advertises in other channels. fennecbot similarly runs in mobile while it advertises in several other channels. Should this bot be different? |
|
hi @whimboo ! |
| @@ -10,12 +10,15 @@ var ircServer = config.server, | |||
| autoRejoin: config.autoRejoin, | |||
| }, | |||
| client = new irc.Client(ircServer, nick, options), | |||
| botChannel = config.channels[0], | |||
There was a problem hiding this comment.
Just name it channel. It won't be anything else than the bot. :)
There was a problem hiding this comment.
node irc uses channel throughout its documentation for function parameters, so it's a little confusing for anyone who's looking at that documentation to use channel as a separate variable name. I've made the change. I wonder what you'd prefer for the node irc function parameters, which I'm calling channl for now.
There was a problem hiding this comment.
By default we use an a prefix for function parameters across all of our repositories. It would be good to refactor the code appropriate, but lets move this out to another issue. This could even be a good first mentored bug for someone. Lets skip that for now.
|
Also I think at the moment the bot waits for an activity in the channel before it updates the topic of the channel when the testday is about to start. Shouldn't this be handled with a timer event or such? |
|
hi @whimboo a biggish update, now using setTimeout to start and end test day, and other updates per your recent feedback thank you! |
| } | ||
| } | ||
|
|
||
| client.addListener('join', function(channel, who){ | ||
| checkTestDay(); | ||
| client.addListener('topic', function (channl, channlTopic, nick) { |
There was a problem hiding this comment.
Please revert this change to the parameter names. Stripping of chars should never be an option.
|
Hi @whimboo! I've rebased and added test and qa config files. I've left this version of the bot running using the test config, with a test Test Day scheduled for 6A-10A UTC. I'll check that it starts before I go to sleep, and maybe you'll have a chance to check that it's still running fine in the morning CET. Thank you! |
| "autoRejoin": "true", | ||
| "admins": ["ashughes","whimboo","galgeek","bara"], | ||
| "helpers": ["ashughes","whimboo","bara"] | ||
| } |
There was a problem hiding this comment.
Please remove that file from the patch. It's nice for testing but does not belong to the repository.
|
Hi @whimboo! I've removed the testing configs from the repo and updated .gitignore, and I've updated the PR. I'm leaving any additional changes to updateTestDayData for later. Thank you! |
| @@ -1,3 +1,4 @@ | |||
| *.swp | |||
| .DS_Store | |||
|
|
|||
| config.json.test | |||
| config.json.qa | |||
There was a problem hiding this comment.
Those are your own personal files on your machine. This is not for general use. So please remove.
|
Hi, @whimboo! As we noted in IRC, I've updated this PR, backing out the .gitignore edits. Thank you! Barbara |
|
This PR needs to be updated due to merge conflicts. |
|
Hi @whimboo! I've updated. Thank you again for all your help! |
|
This looks good to me. I rebased the commits and merged the branch as e38ec3a. |
hi @whimboo !
with apologies for the untidy commit list, suspecting this requires re-forking the repo to fix, and not sure what that would do to existing pull requests. This will fix issue #31.