Skip to content

Conversation

rce
Copy link
Contributor

@rce rce commented Feb 4, 2016

This was requested by my friends. Also closes #30.

Hopefully I'll have the time to get more contributions coming!

lib/bot.js Outdated

const withMentions = text.replace(/@[^\s]+\b/g, m => {
const user = this.discord.users.get('username', m.substring(1));
return user ? user.mention() : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be return user ? user.mention () : m? I.e. not 0. Also, could probably rename m to match or something more readable.

@ekmartin
Copy link
Member

ekmartin commented Feb 4, 2016

Great! Do you think you could write a couple of tests for this too? Feel free to ask here or PM me on Discord (username ekmartin, server link here: http://www.reactiflux.com/) if you have any questions.

By the way, over at slack-irc (the application this is based on) we try to prefix usernames with an @ if they're valid Slack usernames - that is, they're a member of the Slack channel the message will be sent to. Relevant pull request: ekmartin/slack-irc#68

Updated code: https://github.com/ekmartin/slack-irc/blob/master/lib%2Fhelpers.js#L30-L51
https://github.com/ekmartin/slack-irc/blob/master/lib%2Fbot.js#L217-L223

Could be something to think about for another pull request later on. This is more important first though, what I'm mentioning here is more of a nice to have.

@rce
Copy link
Contributor Author

rce commented Feb 18, 2016

@ekmartin I added some tests! Hopefully they are fine.

I may look into adding @-prefixes later.

constructor() {
super();
this.users = {
get: sinon.stub()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be a sandbox stub, like the ones defined here? https://github.com/rce/discord-irc/blob/irc-to-discord-mentions/test/bot.test.js#L29 That way it gets reset after each test, here: https://github.com/rce/discord-irc/blob/irc-to-discord-mentions/test/bot.test.js#L38

@ekmartin
Copy link
Member

Other than my comment it looks great! :)

@rce
Copy link
Contributor Author

rce commented Feb 24, 2016

@ekmartin Using sandbox.stub() now!

@ekmartin
Copy link
Member

Great, thanks!

ekmartin added a commit that referenced this pull request Feb 24, 2016
Mention discord users in messages sent from IRC
@ekmartin ekmartin merged commit 664e38c into reactiflux:master Feb 24, 2016
@ekmartin
Copy link
Member

Published in 0.6.0 :)

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.

Mentions from IRC -> Discord

2 participants