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 setting to disable IRC- and/or Discord-style mentions #537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Gargaj
Copy link

@Gargaj Gargaj commented Mar 20, 2020

This fixes #528 which is really annoying.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 97.175% when pulling 816c3db on Gargaj:master into 41f8444 on reactiflux:master.

@Gargaj
Copy link
Author

Gargaj commented Mar 20, 2020

@CVSoft in case you still need it

@Gargaj Gargaj changed the title Add setting to disable irc- or discord-style mentions Add setting to disable IRC- and/or Discord-style mentions Mar 20, 2020
@Gargaj
Copy link
Author

Gargaj commented Mar 20, 2020

@Throne3d I know you no longer add code, but this is merge-ready, if you want :)

@SparxySys
Copy link
Collaborator

This seems simple enough for me - if there is enough interest it should probably be merged, although I'm not really a fan of the names of the new config options.

You should, however, update the README as well, advanced options under Example configuration.

@Gargaj
Copy link
Author

Gargaj commented Mar 23, 2020

I admit I struggled with the option names; suggestions welcome? :/

@SparxySys
Copy link
Collaborator

Naming wise, try to KISS: it could be as simple as allowMentionsWithAtPrefix and allowMentionsWithoutPrefix.

However, those names have made me start thinking and propose an alternate solution. I would love your input.

(This does not mean I do not approve of your current PR, it solves a problem aptly).

This is what this current PR does:

The problem you want to solve is:

  • Currently, the bot will try to resolve mentions even for normal words because mentions require no prefix

Your solutioning is:

  • Introduce a configuration setting to disable mentions without a prefix

As a nice-to-have that matches with it:

  • Introduce a configuration setting to disable mentions with a prefix.

Possible alternative solution

Now, I'm just going to 'go wild' here and toss out a different possible approach - I would like your input on it as well:
Problem:

  • The bot will resolve who to mention on words without a prefix, and words with prefix @, and this cannot be changed

Proposed solutioning

  • Allow the configuration of prefixes that are used to resolve mentions.

I would imagine the default configuration could look something like this:

{
    "mentionPrefixes": [
        "",
        "@"
    ]
}

Conclusion

Just some food for thought. This would offer a lot more flexibility in the configuration instead of creating a hard-coded case for every commonly-asked for (combination of..) prefixes (or lack thereof) to mention people with.

@Gargaj
Copy link
Author

Gargaj commented Mar 27, 2020

The problem is that "prefix" is misleading, the format it's using isn't just @nick, but also nick: and nick,.

@Gargaj
Copy link
Author

Gargaj commented Mar 27, 2020

Perhaps instead of mentionPrefixes it should be mentionRegexps?

@jesigloot
Copy link

To throw my apprecation into this; Needed exactly this feature, so compiled it into a docker image myself to run it. 😅

@Throne3d
Copy link
Collaborator

Throne3d commented May 7, 2020

Hey! Sorry to not have responded in a while – I haven't been tracking this repository very well.

I really appreciate the contribution, especially the added tests. I see that there's still some open discussion here, so I'm not going to merge it immediately. (I know it's not a huge difference either way, but still.) If this comes to a consensus I can try to get to merging it then!

@Gargaj
Copy link
Author

Gargaj commented May 7, 2020

For what it's worth we've been running with this for one of our bots and it's working brilliantly.

Base automatically changed from master to main January 22, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make IRC->Discord notifying without '@' prefix behavior configurable
5 participants