core: perform commands on connect

[HumorBaby]

This PR is meant to address #1455. It adds a core setting that is a ListAttribute of commands to perform upon successful connection with the IRC server. These commands are executed at the end of the startup sequence (set MODEs, JOIN channels, and now, perform commands).

It depends on #1460.

Example setting

perform_commands = NICKSERV identify testbot ThisIsMy\,complexP@$sW0RD!!,JOIN #testing,PRIVMSG #testing :I am here!!

raw.log, as expected:

...
>>1553928396.52 NICKSERV identify testbot ThisIsMy,complexP@$sW0RD!!
>>1553928396.53 JOIN #testing
>>1553928396.53 PRIVMSG #testing :I am here!!
<<1553928396.57 :testbot!sopel@pumba.local JOIN #testing * :Sopel: https://sopel.chat
>>1553928396.57 TOPIC #testing
>>1553928396.57 WHO #testing a%nuachtf,387
<<1553928396.57 :e9b9f479e8eb.example.net 353 testbot = #testing :@testbot
<<1553928396.57 :e9b9f479e8eb.example.net 366 testbot #testing :End of /NAMES list.
<<1553928396.57 :e9b9f479e8eb.example.net 331 testbot #testing :No topic is set.
<<1553928396.57 :e9b9f479e8eb.example.net 352 testbot #testing sopel pumba.local e9b9f479e8eb.example.net testbot H@ :0 Sopel:
https://sopel.chat
<<1553928396.57 :e9b9f479e8eb.example.org 315 testbot #testing :End of /WHO list.
<<1553928396.61 :NickServ!NickServ@services.int NOTICE testbot :You are now identified for testbot.
...

Note:

• There is no space around commas delimiting commands
• Only the comma of the password is escaped; all other characters do not need to be escaped.

Why [core]?
I had considered two alternatives to adding perform_commands directly into core:

1. Make perform its own module

• Pros
  • Get its own config section
  • Easily add the list, swap, add, del commands from ZNC's perform as commands that the bot's admin can use
  • No need to modify the core bot
• Cons
  • Would have to do something like: set @interval on a function that checks if bot.connection_registered; once connected, execute the commands; remove the @interval function from the job scheduler (from within the same function itself)
  • The above approach would go crazy on .reload without additional measures to track the execution status of commands

2. Introduce a signal/slot system to connect the successful startup event (basically represented by the startup function to perform actions)

• Pros
  • Same as (1), except modifying the core bot part
• Cons
  • Need to modify the core bot to add signals
  • Don't even get me started on .reload 💥... it's bad enough register/unregistering commands
  • I imagine something like this will be in the works soon anyway 😄 🤞, so have a dedicated PR for that, instead of this being introduced as a by-product of this PR.

Plans
I hope a solution like (2) can be achieved at some point. With all the changes to the module loading interface coming along, I think it would be reasonable (and relatively simple) to migrate this to its own module.

[dgw]

Looks like there's some amount of "wonk factor" between Travis and draft PRs still, leading to this PR not getting built when you marked it ready for review. As it happens, that won't be an issue this time because I have things I'd like changed. (And you'll probably rebase this anyway after #1460 goes in—up to you if you address feedback now or wait until I merge that… Soon™.)

[HumorBaby]

(And you'll probably rebase this anyway after #1460 goes in—up to you if you address feedback now or wait until I merge that… Soon™.)

@Exirel pointed out that this would be best re-implemented after the new plugin interface goes in anyway 😄 Addressed the requested changes still, because, why not?

[Exirel]

@HumorBaby this need to be rebased since the PR it requires has been merged.

About the feature itself... I'm not super happy about it. I feel like we are trying to provide a half-baked hook into Sopel to help admin users implement a feature Sopel does not handle yet.

So... meh? Maybe this comment should be in #1455 though...

[dgw]

I feel like we are trying to provide a half-baked hook into Sopel to help admin users implement a feature Sopel does not handle yet.

My perspective on this is, we aren't going to add support for every little feature on every obscure IRCd or network. Sending arbitrary commands at connect is frequently requested, and is (anecdotally) supported by most other popular IRC clients.

[HumorBaby]

So I made quite a few changes on top of the last requested changed:

1. added tests (this required modifying MockSopel)
2. updated docstrings based on more recently used conventions
3. added a relevant section to the dedicated core configuration page
4. added $nickname expansion to commands

Hopefully (4) was the only un-solicited/-desired/-expected change after what seemed like a completely trivial requested change in the review 😅. Since I was rebasing on master anyway, I squashed/reordered previous commits as necessary.

[dgw]

I don't have much to say on this one. The tests look solid, which means the logic looks solid, and all that's left is to decide which gets merged first: this or #1628. I'd like to pull in the config changes first, but I fear that that invites further delay of this because @HumorBaby often doesn't have time to update PRs for days or weeks at a time.

HB, do you mind leaving this for one more go-around after the newline stuff goes in?

Addressed. Final approval pending another PR's state.

[dgw]

This is one of only three "review required" PRs not authored by me. One is deferred until Sopel 8, one is just so big I haven't had time to look at it, and this one has a merge conflict now after waiting so long for #1628 to tweak how ListAttributes get stored.

@HumorBaby do you need to make any updates to this, besides the merge conflict? Or is it drop-in compatible with the ListAttribute changes?

[HumorBaby]

@HumorBaby do you need to make any updates to this, besides the merge conflict? Or is it drop-in compatible with the ListAttribute changes?

Should be a drop in. Will confirm, of course.

[dgw]

Docs too, I hope. Any examples should use the new style, for example.

[dgw]

Needs updates to documentation, as described (and predicted) by the TODO comments left in.

[Exirel]

So, I've no idea how I can push my commits to @HumorBaby 's branch. So I've my own branch, available here: master...Exirel:feat-perform-commands-on-connect

It contains an up-to-date version of this branch, rebased upon the latest sopel/master branch, with the tests modified to fit with the new tests tools.

[dgw]

@Exirel short answer: git push -f HumorBaby feat-perform-commands-on-connect, assuming you added HB's remote with that name.

EDIT: So I don't forget later, this should be able to drop the bot.write patch commit, since the testing mocks have been redone.

[Exirel]

EDIT: So I don't forget later, this should be able to drop the bot.write patch commit, since the testing mocks have been redone.

And also fix the PR so it can be merged. I'll work on that later (probably tomorrow morning).

[Exirel]

@dgw rebased upon master, docstrings fixed, and _execute_perform is now properly private.

[dgw]

Thank you @HumorBaby for taking this so far!

And thank you @Exirel for finishing the last bits (and tweaking it for your own test-suite changes 😛)!

[dgw]

At some point we'll standardize the indent for blocks like this (I tend to use 4 because I just press Tab in my editor), but I'm not going to quibble over it until we actually have a documentation style guide. Writing one is on my to-do list for 7.1-ish.