search

ekmartin / slack-irc

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

Notice option #54

Closed robertkety closed 9 years ago

robertkety commented 9 years ago

Added optional messages for join, part, and quit as requested in comments on pull request #41 (Updated IRC support).

ekmartin commented 9 years ago

I'm not quite sure about this one, but I'd very much be interested in other opinions here. I could see it being helpful for smaller channels, but even there I'm afraid it could be a bit spammy. If people see it as useful I don't mind having a couple extra config options, but no point in extra bloat if not people are going to use it. What do you think @andebor?

In regards to the pull request, is there any point in differentiating between quits and parts? Listening to both events is needed, but I think one message/config value to show people leaving channels is enough. Also, any reason for grouping the options under slackOptions?

By the way, what happened with the notice support from #41? If you're still interested it'd be nice with a PR fixing that (with added tests), so we can close #39. https://github.com/ekmartin/slack-irc/pull/41/files#diff-7202bb7fb017faefd425a2af32df2f9dR88

ekmartin commented 9 years ago

I added notice support when working on actions earlier today, released in 3.6.0, so you can ignore my last paragraph.

robertkety commented 9 years ago

I can see where this would be spammy in a large IRC channel, but these are optional and default behavior is disabled. Small channels would find it more useful. As a matter of fact, it's the primary reason I'm using this solution. We can group quits and parts, I don't have a problem with that. My initial thought was to differentiate options based on action, allowing for greater customization of this solution through the configuration file. However, these actions are close enough in purpose to combine them in the options. Is there a reason you're pushing for a simpler or limited configuration file?

ekmartin commented 9 years ago

Sorry for the late answer. My argument against a more complex configuration file is simply that the problem slack-irc solves isn't complex, and thus shouldn't require that much reading to setup.

The reason I'm a bit unsure about the slackOptions is that ircOptions are passed directly to node-irc (maybe it should be renamed to nodeIrcOptions or similar), which is quite different from slackOptions. Not sure if joinNotices and leaveNotices/partNotices is enough, or if it should be renamed to something like ircJoinNotices. What do you think?

Also, I guess a discussion point would be if it should be possible to do the same for Slack -> IRC. Not sure if it's that useful though, since that happens a lot less often than with IRC join/parts.

If you want to I'd love if you added some tests too, see https://github.com/ekmartin/slack-irc/commit/fced1b52756cc544e08a2f015e09064939d03492 for inspiration. If you don't have time I'll do it before merging it in. Looks like the branch needs to be rebased on master too, as it's not mergable at the moment.

mxm commented 9 years ago

:+1: I was also looking for this feature. I share @ekmartin's concerns on the current approach to configuring the messages.

I'm not really familiar with Slack but would it be possible to grey-out the join/leave messages or make them smaller. That way they might look less spamy..

robertkety commented 9 years ago

The division of options into two objects was a reflection of the two modes of communication that the bot uses. Bot to irc and bot to Slack. This is simply a clean code issue. Making logical divisions early will improve readability in the future. The same holds true for the quit/part combination. My concern is that lumping all options into a single object would be sloppy. Someone might want to differentiate between part/quit. That's why they remain separate in both the irc standard and node-irc implementation. As for naming convention, I really don't mind if someone changes the names on these variables. I'll consider adding some tests when I have time.

On Wed, Sep 23, 2015 at 8:19 AM, Martin Ek < notifications@github.com [notifications@github.com] > wrote: Sorry for the late answer. My argument against a more complex configuration file is simply that the problem slack-irc solves isn't complex, and thus shouldn't require that much reading to setup.

The reason I'm a bit unsure about the slackOptions is that ircOptions are passed directly to node-irc (maybe it should be renamed to nodeIrcOptions or similar), which is quite different from slackOptions . Not sure if joinNotices and leaveNotices / partNotices is enough, or if it should be renamed to something like ircJoinNotices . What do you think?

Also, I guess a discussion point would be if it should be possible to do the same for Slack -> IRC. Not sure if it's that useful though, since that happens a lot less often than with IRC join/parts.

If you want to I'd love if you added some tests too, see fced1b5 [https://github.com/ekmartin/slack-irc/commit/fced1b52756cc544e08a2f015e09064939d03492] for inspiration. If you don't have time I'll do it before merging it in. Looks like the branch needs to be rebased on master too, as it's not mergable at the moment.

— Reply to this email directly or view it on GitHub [https://github.com/ekmartin/slack-irc/pull/54#issuecomment-142637112] .[https://github.com/notifications/beacon/AF6-kPgL558395C4PP9MehcAg6PJXm_Aks5o0rpogaJpZM4F8hGi.gif]

robertkety commented 9 years ago

Moved to #59