-
Notifications
You must be signed in to change notification settings - Fork 293
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
Bridge IRC join/part/quit messages to Discord #207
Conversation
Hmm, coverage decreased somewhere. I'll look into fixing that. |
It looks like the only lines missed now are |
The test description of |
lib/bot.js
Outdated
@@ -98,6 +100,24 @@ class Bot { | |||
this.sendToDiscord(author, to, `*${text}*`); | |||
}); | |||
|
|||
this.ircClient.on('join', (channel, nick) => { | |||
if (this.disableJoinPartMsgs) return; | |||
if (nick === this.nickname && !this.announceSelfJoin) return; |
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.
Didn't know you'd receive join events for yourself. Do we need a configuration option for this though? There's already quite a lot of configuration possible, so maybe it would be better to just keep this off.
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.
Personally, I found this useful to have on. If the bot had to reconnect for some reason (e.g. config change, lost connection to IRC), the self join message acts as a ping to others in the channel, that the IRC bridge is re-established and ready.
That said, it's currently only half-useful as disconnects are not mentioned in-channel, meaning users won't be aware that the bridge is suddenly unavailable for whatever reason. ^1
Edit: I suppose this could be handled as a separate feature, where the bot can itself announce when the bridge is up or down...
^1 This is not obvious in Discord; users don't think to check that the bot is online in the right sidebar
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 see. Could keep it as a non-documented option, but as you're saying I think it might make more sense to print something like The bot has joined the IRC channel.
then. Not sure how you'd be able to announce if the bridge is down though, as I think most issues with crashes have been on the Discord side (i.e. timing out and not reconnecting), but I guess you could at least print if the IRC connection goes offline. Seems a bit of an edge case though.
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.
Indeed; in my edge case, I want to avoid the rare instances where users may start having one-sided conversations into the void. But, so far, that has yet to happen. So maybe it is not a big enough deal :)
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.
In that case, wouldn't it make more sense to save up messages and send them when the IRC connection comes online? Maybe not if it's more than say 10 messages, but in your case I guess it could be a useful feature. Might not be worth the extra code though.
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'm not sure what to do in response to this – should I leave it as is for now, or…?
lib/bot.js
Outdated
} | ||
|
||
/* Sends a message to Discord exactly as it appears */ | ||
sendSpecialToDiscord(channel, text) { |
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.
Hm, is there any reason this can't just use sendToDiscord
as well?
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 was mostly just doing this since the other PR did – should I add a boolean flag to display it differently, or…?
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.
Never mind, I was a bit confused since I forgot that we append usernames to the messages even when sent to discord (slack-irc doesn't). The current approach makes sense. Could maybe call it something like sendExactToDiscord
though. Also, getDiscordChannelFor
-> findDiscordChannel
/getDiscordChannel
or something like that?
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.
Renamed to sendExactToDiscord
and findDiscordChannel
.
lib/bot.js
Outdated
@@ -32,6 +32,8 @@ class Bot { | |||
this.commandCharacters = options.commandCharacters || []; | |||
this.ircNickColor = options.ircNickColor !== false; // default to true | |||
this.channels = _.values(options.channelMapping); | |||
this.disableJoinPartMsgs = options.disableJoinPartMsgs; |
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.
Thanks for adding a configuration option! I think it would be better to default it to off though, as that's the current behavior. slack-irc uses an object for this, so you can choose to enable joins and leaves selectively:
I'm not sure if that's really necessary though, a single option might be more than enough. Maybe just ircStatusNotices: true
.
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've converted it to a single ircStatusNotices
config.
(Rebased onto master, rebased to change the config |
Looks good! Wanna add a line to the advanced example configuration in the README with |
- Do not discard reason from part messages - Output to each channel in the quit messages - Add tests for join/part/quit events - Add test that sendSpecialToDiscord doesn't send messages to outside the channel mapping
Done (and fixed a merge conflict after #204 was merged). |
Partially solves #92? |
Thanks! I'll publish a release with the recent changes in the next few days. |
As in PR #40, this bridges the join/part/quit messages from IRC to Discord. It builds on that PR (thanks, @Ratismal), rebasing it onto master (and accidentally doing something stupid in the rebase, which a later commit fixes).
This adds two config settings:
disableJoinPartMsgs
– if set to true, the bot won't announce anyircStatusNotices
– if set to true, the bot will announce join/part/quit messages (if not set, overrides below)announceSelfJoin
– if set to true, the bot will announce itself (by default it skips any join events involving itself)There are tests to ensure it works appropriately, and manual testing seems to show it working well. I hope this PR has more of a chance of getting merged?