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

plugin: add capability.__str__ for better logs #2569

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

half-duplex
Copy link
Member

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

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

@dgw dgw added the Needs Triage Issues that need to be reviewed and categorized label Nov 27, 2023
@SnoopJ
Copy link
Contributor

SnoopJ commented Nov 27, 2023

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
Copy link
Contributor

Exirel commented Nov 27, 2023

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
Copy link
Member

dgw commented Dec 11, 2023

@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 dgw added this to the 8.0.0 milestone Dec 11, 2023
@dgw dgw removed the Needs Triage Issues that need to be reviewed and categorized label Dec 11, 2023
@dgw
Copy link
Member

dgw commented Dec 13, 2023

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

@dgw dgw requested a review from Exirel December 13, 2023 08:36
@dgw dgw merged commit f3c88e6 into sopel-irc:master Dec 14, 2023
15 checks passed
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.

4 participants