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

irc/bot: implement echo-message support #1072

Closed
wants to merge 2 commits into from

Conversation

maxpowa
Copy link
Contributor

@maxpowa maxpowa commented May 2, 2016

Allows modules to listen in on what's being sent by Sopel. Compatible with all the other decorators, as it goes through all the same logic. If echo-message is supported on the server and is toggled on by a module, only functions with the @sopel.module.echo(True) decorator will receive those echo messages. If echo-message isn't enabled, the same behavior still exists but the hostmask will not be as accurate.

@dgw
Copy link
Member

dgw commented May 2, 2016

Awesome! My only thought is a concern that there will be increased risk of modules generating loops if this particular decorator defaults to True. Would it not be better to default False and let only modules that depend on observing the bot's output (loggers, for example) override that?

@maxpowa
Copy link
Contributor Author

maxpowa commented May 2, 2016

Close, but it's default True when decorated -- if you have an undecorated function it won't get echos. It's just when you decorate with @sopel.module.echo() it assumes true.

@dgw
Copy link
Member

dgw commented May 2, 2016

Ah, OK. Still possible for module authors to write confusing code that doesn't explicitly specify True, but old modules that were written before the decorator was available still won't get echos. Just me being an idiot. 👍

@maxpowa
Copy link
Contributor Author

maxpowa commented May 24, 2016

I just realized that this isn't really correct. As it is right now, it will echo every type of message, but the echo message spec only includes NOTICE and PRIVMSG. I will update this PR when I get time.

@embolalia
Copy link
Contributor

I like the idea, like seriously. This is awesome and has been desired for ages. But I don't think the @echo decorator should take a value. The default should be definitely not to get echoes, meaning the decorator only needs to mean "yup this thing can get echo messages". But other than that, this is the perfect solution.

@dgw dgw added this to the 7.0.0 milestone Mar 23, 2018
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

If @maxpowa is willing to address these in the (probably long) time between now and when Sopel 7 comes, that's great. If not, I will simply add them on in a new commit, without squashing so the feature's history remains visible.

@@ -484,6 +484,9 @@ def dispatch(self, pretrigger):
if (hasattr(func, 'intents') and
trigger.tags.get('intent') not in func.intents):
continue
if (not (hasattr(func, 'echo') and func.echo is True) and
trigger.nick.lower() == self.nick.lower()):
continue
Copy link
Member

Choose a reason for hiding this comment

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

I'd put this check first in the enclosing for loop, but the order isn't really an issue tbh.

if self.config.core.bind_host:
host = self.config.core.bind_host
pretrigger = PreTrigger(self.nick, ':{0}!{1}@{2} {3}'
.format(self.nick, self.user, host, temp))
Copy link
Member

Choose a reason for hiding this comment

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

This line made me notice the overuse of a variable named temp in this function. Renaming that to something sensible should happen after this is merged.

@@ -115,6 +115,23 @@ def add_attribute(function):
return add_attribute


def echo(value):
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @embolalia:

I don't think the @echo decorator should take a value. The default should be definitely not to get echoes, meaning the decorator only needs to mean "yup this thing can get echo messages".

This decorator should just set the echo function attribute to True if called. That's all it needs to do.

@@ -148,6 +148,16 @@ def write(self, args, text=None):
self.send(temp.encode('utf-8'))
finally:
self.writing_lock.release()
# Simulate echo-message
if 'echo-message' not in self.enabled_capabilities:
Copy link
Member

Choose a reason for hiding this comment

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

As @maxpowa mentioned in the main PR comments, "As it is right now, it will echo every type of message, but the echo message spec only includes NOTICE and PRIVMSG." Therefore, this block should only fire if the message is one of those types.

@dgw
Copy link
Member

dgw commented Jan 25, 2019

@maxpowa Would you be willing to allow edits from maintainers on this PR? I just rebased it, but can't push the updated history back here (probably because the PR is so old).

@dgw
Copy link
Member

dgw commented Feb 4, 2019

Closing in favor of rebased/updated version (#1470).

@dgw dgw closed this Feb 4, 2019
@dgw dgw removed this from the 7.0.0 milestone Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants