ekmartin / slack-irc

Connects Slack and IRC channels by sending messages back and forth.
MIT License
588 stars 157 forks source link

Add slackUsernameFormat config option #171

Closed laughinghan closed 7 years ago

laughinghan commented 7 years ago

Close #56

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.003%) to 99.457% when pulling 4fb44ed4a4846819d41f281791fca82ae5485502 on laughinghan:slackUsernameFormat into d35d7b9c0bb68b90ffe77ec33104300b0f88f111 on ekmartin:master.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.003%) to 99.457% when pulling 4fb44ed4a4846819d41f281791fca82ae5485502 on laughinghan:slackUsernameFormat into d35d7b9c0bb68b90ffe77ec33104300b0f88f111 on ekmartin:master.

laughinghan commented 7 years ago

@ekmartin: lol did you add Coveralls twice or something

ekmartin commented 7 years ago

Brilliant! I agree with what you said in https://github.com/ekmartin/slack-irc/issues/56#issuecomment-258967286 - let's go with $username (IRC) as the default. Whether it's a breaking change or not is up for discussion, but I think it's fine to just do it in a minor version bump, since as you said - it's a cosmetic change.

Should this maybe be nested under an object, to make it cleaner to add support for IRC username formatting as well?

"usernameFormat": {
   "slack": "$username (IRC)",
   "irc": "<$username>"
}

Or do you think it's clearer with just slackUsernameFormat and ircUsernameFormat?

EDIT: No idea what's happening with coveralls, let's see if it's a one-time incident ¯\_(ツ)_/¯

laughinghan commented 7 years ago

Great! Will do.

I don't think it should be nested, but I also see no use for ircUsernameFormat since messages in IRC from the bot (piped from Slack) are already distinguished by coming from the bot, much moreso than bot messages in Slack.

ekmartin commented 7 years ago

I think one use case would be if people for some reason would want to remove the username when messages are posted to IRC. Might be good to have both just for unity, but we can merge this and look at it later.

laughinghan commented 7 years ago

@ekmartin: eh yeah let's just deal with that later. Updated default Slack username format.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.003%) to 99.457% when pulling 233480070983d9d09083e79c86392d9ff1c972da on laughinghan:slackUsernameFormat into d35d7b9c0bb68b90ffe77ec33104300b0f88f111 on ekmartin:master.

ekmartin commented 7 years ago

Great, thanks! I'll create a release later.

ekmartin commented 7 years ago

Released in 3.9.0 - thanks again!