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 #1470

Merged
merged 8 commits into from
Apr 5, 2019
Merged

irc/bot: implement echo-message #1470

merged 8 commits into from
Apr 5, 2019

Conversation

dgw
Copy link
Member

@dgw dgw commented Feb 4, 2019

Rebased & tweaked version of #1072, which stalled. Resolves #526. Original PR description by @maxpowa follows; see #1072 for the earlier evolution of this patch.


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 dgw added the Feature label Feb 4, 2019
@dgw dgw added this to the 7.0.0 milestone Feb 4, 2019
Copy link
Member Author

@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.

I'm not allowed to mark this as "Request changes" because GitHub has decreed that PR authors aren't allowed to admit when their work is incomplete… but these are the comments I left on the original PR (#1072), brought over to this one so I don't forget to address them when I have the time.

sopel/irc.py Outdated Show resolved Hide resolved
sopel/module.py Outdated Show resolved Hide resolved
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 Author

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.

sopel/bot.py Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Feb 20, 2019

Also want to remind myself, when I come back to this, that at least the find module's collectlines() callable would benefit from @echo. Users could then do Sopel: s/foo/bar/ and have it work as expected. (Yes, my users try to do this sometimes. I do, too, and am always disappointed when it does nothing.)

@dgw
Copy link
Member Author

dgw commented Apr 4, 2019

Implemented the pending feedback from before:

  • Remove argument from module.echo() decorator and just always set the attribute to True on decorated functions
  • Only simulate echo-message for PRIVMSG and NOTICE lines

My tweaks should get a review from one or more @sopel-irc/rockstars before merging 🚀

Also updated the name of temp while I was in here, but that caused merge conflicts that I don't feel like dealing with today, so I removed that commit.

Copy link
Contributor

@HumorBaby HumorBaby left a comment

Choose a reason for hiding this comment

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

The main decorator needs to be fixed.

sopel/module.py Outdated Show resolved Hide resolved
Copy link
Contributor

@HumorBaby HumorBaby left a comment

Choose a reason for hiding this comment

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

Good catch on the default value of echo.

/me approves

@dgw
Copy link
Member Author

dgw commented Apr 4, 2019

Cool, I'mma squash that fixup now that you've seen it.

  - Remove argument from module.echo() decorator and just always set
    the attribute to `True` on decorated functions
  - Only simulate `echo-message` for PRIVMSG and NOTICE lines
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Looking good!

sopel/bot.py Outdated Show resolved Hide resolved
sopel/irc.py Outdated Show resolved Hide resolved
dgw added 4 commits April 4, 2019 13:53
If a function is not decorated with @sopel.module.echo, it should not
throw an exception if code somewhere tries to check `func.echo`'s value.

Simplified check in `bot.dispatch()` with this: since `func.echo` will
always at least exist, there's no need to do any `hasattr()` first.
This landed on master, so we can safely use it in the echo-message
branch assuming all tests are run on merges (which they are in CI).
Corrected misspelled method names related to receiving CAP messages, and
added `echo-message` to the list of core capabilities Sopel should
enable if available from the server (for the module.echo decorator).
@dgw dgw requested review from Exirel and HumorBaby April 4, 2019 22:24
Copy link
Contributor

@HumorBaby HumorBaby left a comment

Choose a reason for hiding this comment

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

🏅 on the spelling correction... the @echo feature is a nice touch too, I guess 😉

@dgw dgw self-assigned this Apr 4, 2019
@Exirel
Copy link
Contributor

Exirel commented Apr 4, 2019

As far as I'm concerned, LGTM. I didn't test it live, but the code-style and the overall logic check out.

@dgw
Copy link
Member Author

dgw commented Apr 5, 2019

Well, I confirmed via raw logs that the echo-message capability is requested, but all of the servers I tried to test on NAK it.

It's hard to find a network that actually supports this. The client capability statistics for "curiosity's sake" at stats.ircdocs.horse show a measly 5 networks (out of 270 surveyed at last update) advertising support for the echo-message capability.

The logic added in this PR should "just work" whether or not the echo-messages are real, but I would like to test at least briefly on a network that actually does echo messages back. Stay tuned, I guess.

dgw added a commit that referenced this pull request Apr 5, 2019
bot.write() joins the elements of a sequence type (tuple, here) with a
space already, so including a space in `'MODE '` caused Sopel to send
`MODE  SopelsNick +modes`. Technically, that's invalid.

Discovered while trying to test #1470 on a network with actual support
for `echo-message`, when darwin.network disconnected the bot during
registration with the message "ERROR :Received malformed line". Annoying
because I was trying to test something else, but useful in the end.
Use new `@module.echo` decorator to collect Sopel's own lines as well,
so cheeky users can say, "Sopel: s/you made/a mistake/" to the bot.

Basically, proof that the echo decorator does its job.
@dgw
Copy link
Member Author

dgw commented Apr 5, 2019

Once I realized I could just look for a network running Oragono (one of few IRCds with echo-message support), it was easy to test this. I even found another unrelated bug along the way (#1544).

Updated find module to collect Sopel's own lines, just for fun.

@dgw dgw merged commit 95812b5 into sopel-irc:master Apr 5, 2019
@dgw dgw deleted the echo-message branch April 5, 2019 01:32
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.

4 participants