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

Support using discord channel ids #187

Merged
merged 4 commits into from
Mar 29, 2017
Merged

Conversation

mraof
Copy link
Contributor

@mraof mraof commented Feb 4, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.351% when pulling b4aef61 on mraof:patch-1 into 8de8f49 on reactiflux:master.

@ekmartin
Copy link
Member

Would you care to explain why you think is useful as well? Can't see any immediate gain, but I might be missing something.

@mraof
Copy link
Contributor Author

mraof commented Feb 15, 2017

It allows the config to keep working even if discord channels are renamed, which is the problem I was having.
It might also allow specifying which channel if the bot is in multiple servers that have a channel named the same, but I haven't tested that

@ekmartin
Copy link
Member

Ye, that makes sense. Care to add a test or two?

lib/bot.js Outdated
@@ -193,9 +194,9 @@ class Bot {
const discordChannelName = this.invertedMapping[channel.toLowerCase()];
if (discordChannelName) {
// #channel -> channel before retrieving and select only text channels:
const discordChannel = this.discord.channels
const discordChannel = discordChannelName.charAt(0) === '#' ? this.discord.channels
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discordChannelName.startsWith('#')

lib/bot.js Outdated
@@ -154,7 +154,8 @@ class Bot {
if (author.id === this.discord.user.id) return;

const channelName = `#${message.channel.name}`;
const ircChannel = this.channelMapping[channelName];
const ircChannel = this.channelMapping[message.channel.id in this.channelMapping ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const ircChannel = this.channelMapping[message.channel.id] || this.channelMapping[channelName] maybe?

@mraof
Copy link
Contributor Author

mraof commented Feb 17, 2017

I'm afraid I don't actually know how to add tests

@coveralls
Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage remained the same at 97.351% when pulling b6f53cb on mraof:patch-1 into 8de8f49 on reactiflux:master.

@Sanqui
Copy link
Contributor

Sanqui commented Mar 16, 2017

I just ran an issue when joining the bot onto two Discord servers with #general. Would appreciate this a lot. Might give writing tests a shot but I'm not familiar with it at all.

@Throne3d
Copy link
Collaborator

I've had a go at adding tests for this in PR #202. Thanks for putting in the work for the functionality.

@ekmartin ekmartin merged commit b6f53cb into reactiflux:master Mar 29, 2017
ekmartin added a commit that referenced this pull request Mar 29, 2017
Add Discord channel ID matching (PR #187) and tests for it
@ekmartin
Copy link
Member

Closing this since #202 was merged. Thanks!

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

Successfully merging this pull request may close these issues.

5 participants