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

Add the ability to ignore IRC/Discord users by nickname #322

Merged
merged 4 commits into from
Oct 4, 2017

Conversation

anirbanmu
Copy link
Contributor

  • Added simple tests
  • All tests passing
  • Did manual verification as well

This implements the simpler part of #236

@coveralls
Copy link

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+0.04%) to 98.94% when pulling be281c5 on anirbanmu:ignore-users-both-ways into 7fbf1d0 on reactiflux:master.

lib/bot.js Outdated

// Discord nicks to ignore (case insensitive)
this.discordIgnoreUsers = options.discordIgnoreUsers || [];
this.discordIgnoreUsers = this.discordIgnoreUsers.map(x => x.toLowerCase());
Copy link
Member

Choose a reason for hiding this comment

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

maybe just compare i.toLowerCase() === item.toLowerCase() instead of changing the array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

lib/bot.js Outdated
@@ -37,6 +41,14 @@ class Bot {
this.ircStatusNotices = options.ircStatusNotices;
this.announceSelfJoin = options.announceSelfJoin;

// IRC nicks to ignore (case insensitive)
this.ircIgnoreUsers = options.ircIgnoreUsers || [];
Copy link
Member

@ekmartin ekmartin Oct 4, 2017

Choose a reason for hiding this comment

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

could we change the config to be an object?

"igoreUsers": {
  "irc": [],
  "discord": [],
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, fixed.

lib/bot.js Outdated
@@ -11,6 +11,10 @@ const NICK_COLORS = ['light_blue', 'dark_blue', 'light_red', 'dark_red', 'light_
'dark_green', 'magenta', 'light_magenta', 'orange', 'yellow', 'cyan', 'light_cyan'];
const patternMatch = /{\$(.+?)}/g;

function caseInsensitiveExists(item, itemList) {
return itemList.some(i => i === item.toLowerCase());
Copy link
Member

Choose a reason for hiding this comment

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

think you might as well inline this in ignoredIrcUser and ignoredDiscordUser - the extra layer of indirection probably just makes it harder to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

lib/bot.js Outdated
@@ -362,6 +387,11 @@ class Bot {
ircChannel: channel
};

// Do not send to Discord if this user is on the ignore list.
if (this.ignoredIrcUser(author)) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe move this check to https://github.com/anirbanmu/discord-irc/blob/be281c5addff12a250866b351a05b91cb9d168e6/lib/bot.js#L377?

either in the same conditional or the line below - up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

@ekmartin
Copy link
Member

ekmartin commented Oct 4, 2017

Other than the comments this looks good to me - thanks!

@anirbanmu
Copy link
Contributor Author

Fixed everything regarding the comments. Please take another look when you have a chance! Thanks! @ekmartin

@coveralls
Copy link

coveralls commented Oct 4, 2017

Coverage Status

Coverage increased (+0.04%) to 98.932% when pulling 94d9096 on anirbanmu:ignore-users-both-ways into 7fbf1d0 on reactiflux:master.

@ekmartin ekmartin merged commit 3bd3220 into reactiflux:master Oct 4, 2017
@ekmartin
Copy link
Member

ekmartin commented Oct 4, 2017

Great - thanks!

@ekmartin ekmartin mentioned this pull request Oct 4, 2017
@@ -90,6 +90,10 @@ First you need to create a Discord bot user, which you can do by following the i
// with one of these characters (commands):
"commandCharacters": ["!", "."],
"ircStatusNotices": true // Enables notifications in Discord when people join/part in the relevant IRC channel

Choose a reason for hiding this comment

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

you missed a trailing comma here

@@ -90,6 +90,10 @@ First you need to create a Discord bot user, which you can do by following the i
// with one of these characters (commands):
"commandCharacters": ["!", "."],
"ircStatusNotices": true // Enables notifications in Discord when people join/part in the relevant IRC channel
"ignoreUsers": {
"irc": ["irc_nick1", "irc_nick2"] // Ignore specified IRC nicks and do not send their messages to Discord.

Choose a reason for hiding this comment

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

another one here

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! Fixed in master.

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.

4 participants