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

Deprecate usage of bot.msg to bot.say #1606

Merged
merged 2 commits into from
May 15, 2019

Conversation

deathbybandaid
Copy link
Contributor

This PR aims to deprecate the usage of bot.msg as per:

    def msg(self, recipient, text, max_messages=1):
        # Deprecated, but way too much of a pain to remove.
        self.say(text, recipient, max_messages)

As of right now, anything that calls bot.msg gets passed to bot.say. This skips that step.

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.

This is 90% there; just a couple things that should probably be added/tweaked. I'm actually kind of excited, in some strange way, to get some forward motion on removing this old piece of cruft.


Remaining calls to msg() that should be replaced, too:

sopel/sopel/bot.py

Lines 428 to 429 in 54d451f

if excess:
self.msg(recipient, excess, max_messages - 1)

sopel/sopel/irc.py

Lines 458 to 459 in 54d451f

if trigger and self.config.core.reply_errors and trigger.sender is not None:
self.msg(trigger.sender, signature)

sopel/sopel/irc.py

Lines 463 to 464 in 54d451f

if trigger and self.config.core.reply_errors and trigger.sender is not None:
self.msg(trigger.sender, "Got an error.")

I'm guessing you ran a global search for "bot.say", and these were missed because they get called via self instead of bot. It's important to replace every place where msg() is called because of what I'm about to ask for in part 3 of this change request.


Since this set of changes isn't actually deprecating anything, it would be great if the commit message (once you squash in those last few replacements) could read, "Replace deprecated uses of msg() with say()". This is also to leave room for part 3 of my requested changes¸ which is…


…to make the deprecation of msg() officially official by marking it with the decorator we have for that: @sopel.tools.deprecated—which is not, sadly, documented in the current API docs because it doesn't have a docstring yet in 6.6.x. But it's pretty self-explanatory to use.

This can (and probably should) be done in a separate commit, something like "bot: make deprecation of msg() official". If the method still had a docstring we'd want to mark it with all the special Sphinx goodness, too, but it doesn't. We should be safe enough just decorating it.

Feel free to note in the commit message body that it's been (with the quotes, if you like) "deprecated" since version 6.0, and this is merely making that fact more visible to anyone still using the old method.

If we release Sopel 7 with the code to print a warning when bot.msg() is used, and warn in the release notes (my job) that it finally will be removed in Sopel 8, we can actually get rid of it… finally! (It's been almost 4 years since the desire to remove it was expressed: 1217c40)


Finally (wow, this got longer than I thought it would), we should put the .msg command from admin.py on notice. It's silly enough already having a command that doesn't match the API, but I've been tripping over that one for ages. I inevitably try to use .say instead, and it doesn't work (obviously).

That's something else we should replace, along with the API and code updates. For 7.0, if you want to, add 'say' to the list of commands there (first, since the first command name is what shows in help), and put an appropriate comment reminding us to remove the old 'msg' command name for 8.0. Once again, it'll be my job to make sure that gets called out in the release notes & migration guide for each version.

@dgw dgw added this to the 7.0.0 milestone May 15, 2019
@dgw dgw added the Tweak label May 15, 2019
@Exirel
Copy link
Contributor

Exirel commented May 15, 2019

@dgw I would suggest to do that in a different PR, since self.msg is used in the sopel.irc.Bot class, but this class does not define any msg method, or say method (among with other undefined stuff).

Basically, some of sopel.irc.Bot's methods can not work unless they are called from an instance of sopel.bot.Sopel. And that's super weird, and I would like very much to rework that.

It's already a great achievement to remove all occurrence of bot.msg outside of the Bot class, and should probably get merged right away.

@dgw
Copy link
Member

dgw commented May 15, 2019

Basically, some of sopel.irc.Bot's methods can not work unless they are called from an instance of sopel.bot.Sopel. And that's super weird, and I would like very much to rework that.

If msg isn't defined but works, then say can be undefined but work just the same.

I'd rather replace everything and then you can rework what you want without worrying about merge conflicts. Whether we leave self.msg or update it, it's going to stay weird either way.

I had the same reaction—that it's weird to use stuff from a child class in the parent—but we shouldn't let that stand in the way of killing off bot.msg. It's been deprecated for far too long already. It needs to die.

@deathbybandaid deathbybandaid force-pushed the bot.msg-deprecate branch 2 times, most recently from 59aa34d to 8b95451 Compare May 15, 2019 11:22
@deathbybandaid
Copy link
Contributor Author

assuming I used @deprecate correctly, I've made all the changes suggested.

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.

Neat and straightforward PR. LGTM.

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.

Yep, you used @deprecated exactly as it should be. 🚢 🚀

@dgw dgw merged commit a2e7378 into sopel-irc:master May 15, 2019
dgw added a commit that referenced this pull request Oct 4, 2019
Mixed up by @deathbybandaid in #1606. For shame that the rest of us
missed it in code review, too!
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