sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
951 stars 405 forks source link

plugin: add capability.__str__ for better logs #2569

Closed half-duplex closed 6 months ago

half-duplex commented 7 months ago

Description

When Sopel is started with loglevel DEBUG, it currently outputs a stack of lines like this:

sopel.plugins.capabilities DEBUG   : Capability Request registered: <sopel.plugin.capability object at 0x7f77903c7cd0>

The str(cap_req) gives no useful information. This PR adds capability.__str__() so it does:

sopel.plugins.capabilities DEBUG   : Capability Request registered: <capability 'message-tags'>
sopel.plugins.capabilities DEBUG   : Capability Request registered: <capability 'account-notify' (_handle_account_and_extjoin_capabilities())>

Open to format opinions.

Also, given the format, should this __str__() and that for Rule and Command be __repr__()?

Checklist

SnoopJ commented 7 months ago

Also, given the format, should this __str__() and that for Rule and Command be __repr__()?

It seems to me that the __str__() implemented here could probably be worked into a __repr__() and the log line could be changed to use %r. Might simplify the logic a bit.

Exirel commented 7 months ago

I like the idea. For now the rules have a __str__ but that's probably not what we actually want, and __repr__ could be it.

dgw commented 6 months ago

@Exirel @SnoopJ This seems like a no-brainer for 8.0.0 to me, so its logs are intelligible. We can look at switching the relevant bits to __repr__() in 8.0.1 or 8.1. Any objections?

dgw commented 6 months ago
<+xrl> Not a big fan of the lack of tests in #2569

Well now it has test coverage. I stopped short of hard-coding the exact log message format, but it's enough to make sure that the main concern will be caught:

<+xrl> Yes. Because it is used in the code somewhere and one day it may raise an exception because something changed
       with unexpected consequences.
<+xrl> Happened to me a lot. `__str__` looks innocuous, but it isn't.