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

adminchannel, coretasks: improve handling of bot's current nick #2240

Merged
merged 2 commits into from Jan 27, 2022

Conversation

dgw
Copy link
Member

@dgw dgw commented Jan 21, 2022

Description

Tin, kind of.

There's a bit more esoteric detail in the commit messages if anyone's interested, but the short version is that we really should be moving toward a world where bot.nick always represents the bot's current nick in use—even if that doesn't match the config.

While we aren't there yet, this is a step (or two) forward on that path. I've made it so that coretasks' NICK event handler updates the bot's nick if it changes, and only warns for a change away from the config value.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Notes

On the way to writing this patch, I discovered that bot.change_current_nick() would send a NICK command to the server along with updating the bot._nick value. That's probably a bad idea, actually, and I didn't use it (also because it's redundant to send NICK thenickwealreadyhave). We should have another look at that method and rethink it…

I traced use of the equivalent to `bot.config.core.nick` all the way
back to the 3.x (Phenny) days. Couldn't see a reason why this check
should use the configuration value. It just always has, probably because
`bot.nick` hasn't always meant what it means in the Sopel of today.

Since the only logical reason to check the nick is to prevent someone
from making the bot kick itself, that's what we'll make sure it does.
And since the bot's nick can theoretically change at runtime, the check
should use the bot's *current* nick, not the config value.
It's a good idea to make sure the bot always knows its current nick,
even if that value doesn't match what it wants (from config).

If `echo-message` is available, for example, the server will send lines
using the bot's *current* nick, not the configured one. Sopel simulates
this if the capability isn't enabled. We want e.g. the `plugin.echo`
decorator to still work regardless.
@Exirel
Copy link
Contributor

Exirel commented Jan 21, 2022

We should have another look at that method and rethink it…

Does this mean that the bot should change bot.nick only when the server tells that the nick has been changed? Does the server always reply back to the client after it sends a NICK command?

If yes: sure, we can change that behavior. If no: then we still need this method.

In any case, we also need a different, separate method that only set the current nick, something like this:

def set_current_nick(nick: str) -> None:
    self._nick = self.make_identifier(nick)

And keep change_current_nick() as a way to send a NICK command.

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.

It works as it is. However, if we are heading toward that direction, I suggest to add a set_current_nick() method to the bot (probably on sopel.irc.AbstractBot).

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

dgw commented Jan 21, 2022

The way forward is probably to keep bot.change_current_nick() as-is, add the new bot.set_current_nick() method, and add NICK error handling to revert the bot's internal state if the server rejects a bot.change_current_nick(). I'd love to cleanly separate change and set, but there's no specified behavior in RFCs 1459 or 2812 about what the server should send (if anything) when a client changes nicks except for the error numerics. Modern/horse docs just has a "may" (not even in caps) in reference to servers sending a NICK command back to the originating user confirming the change. Sigh.

This PR is already way beyond the original scope, though, so I'm going to work on that stuff in a new branch based on this.

@Exirel
Copy link
Contributor

Exirel commented Jan 21, 2022

This PR is already way beyond the original scope, though, so I'm going to work on that stuff in a new branch based on this.

Heh, you already got my approval, merge if you feel like it!

@dgw dgw merged commit 6e56fc6 into master Jan 27, 2022
@dgw dgw deleted the adminchannel-kick-check branch January 27, 2022 19:34
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.

None yet

2 participants