search

mozilla / testdaybot

Mozilla QA Test Day IRC Bot
9 stars 12 forks source link

Update node-irc to version 0.3.7 #45

Closed galgeek closed 9 years ago

galgeek commented 9 years ago

Hi @whimboo!

This PR updates node-irc to the current version, including minor updates to testdaybot's bot.js and package.json, addressing issue #39.

0.3.7 fixes the bug in Client.whois and appears to better support Client.join, necessary for PR #38 Advertise Test Day.

galgeek commented 9 years ago

Hello, again @whimboo!

I have this version of the bot, merged with the outstanding PRs, except for Advertise..., running now in the #testdaybotTest channel, if you have a chance to take a look.

Thank you again for all your help!

galgeek commented 9 years ago

Hi, @whimboo!

I've redone this pull request, using npm install to install the node-irc library, v0.3.7, dated May 2014, rather than cloning the git repo (and creating a submodule, which I've realized we don't need/want).

On the list of files changed, the two files outside node-irc that change are bot.js, which resets floodProtection, and package.json, which updates testdaybot's version to 0.2a and specifies this version of node-irc as a dependency.

I've uploaded another branch to my github account that updates node-irc to v.0.3.8, updated in the past couple of days. My install of v0.3.8 depends on what is likely to be an additional install for most (the icu library). This allows v0.3.8 to support character sets that node does not. Both versions seemed fine in my quick tests. I am a little more comfortable trusting v0.3.7, and it should be an easier install.

Thank you for your patience!

whimboo commented 9 years ago

Sure, if version 0.3.7 fixes our crash we are good to go. I wouldn't worry about charset features coming with 0.3.8.

galgeek commented 9 years ago

I'm content with v0.3.7.

I was just wondering whether the additional character set support might be of interest.

whimboo commented 9 years ago

Ok, so I checked locally and I see a couple of differences comparing the current list of files and the ones I get with https://github.com/martynsmith/node-irc/archive/v0.3.7.tar.gz. Please update your PR so it has all the files from the github repository. We can go to official packages in a follow-up issue if really necessary.

galgeek commented 9 years ago

Hi @whimboo!

The only file I see in that .gz that's not in my repo is the .gitignore, using diff -r.

My repo has a couple of files the .gz does not, and some differences in package.json, entirely unremarkable, probably the result of npm packaging.

I wonder if you might be looking at something else?

whimboo commented 9 years ago

As long as we do not use the package manager please ensure to take the archives from github. Don't use the files installed via npm for the time being. This can be done as one of the next steps. Please also make sure to check what's up with the ansicolor package.

galgeek commented 9 years ago

Hi @whimboo!

I've replaced the npm install with the files from the github archive. Those ansi-color files are gone, since they get installed by npm as a dependency. I've tested; this configuration still runs fine for me.

whimboo commented 9 years ago

Looks good to me know. I squashed the commits, fixed the commit message, and merged the PR as https://github.com/mozilla/testdaybot/commit/862b44d70038139ae8a1871006758a1034d80d78.