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

Sopel bind trigger #2443

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Apr 10, 2023

Description

(based on #2441 for simplicity)

For ease of use, Sopel provides a SopelWrapper to plugin callable so they can use bot.say(message) without having to specify the destination: the wrapper will look at the trigger's sender and send the message back to it (be it a channel or a nick).

However, this has the disavantage of having to 1. maintain a separate class with dunder methods (boo, magic!), and 2. it makes static typing more complex.

Welcome Python 3.7 and its new built-in module: contextvars. This wonderful utility module provides a thread-safe and asyncio-safe object that can have a context specific value.

Sopel now uses this feature to bind itself to:

  • a trigger that matches a rule
  • the triggered rule

and can be used in place of a SopelWrapper instance.

The biggest change are on call_rule, which doesn't require anymore a SopelWrapper instance, and on the tests, in particular on the trigger factory's wrapped method, which now returns a Sopel instance.

The second side-effect on test is that now it is not possible to bind a trigger multiple times on a test bot, and tests have to rely on Sopel.sopel_wrapper context manager (as Sopel.call_rule does), which requires a Rule object. This is not perfect, albeit manageable.

I've updated the documentation a bit to reflect that, however I think more should be done to explain all that. The change that matters is in anatomy.rst, that makes this possible:

from sopel.bot import Sopel
from sopel.trigger import Trigger

def plugin_callable(bot: Sopel, trigger: Trigger) -> None:
    bot.say('Hello!')

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

@Exirel Exirel added Tweak Breaking Change Stuff that probably should be mentioned in a migration guide labels Apr 10, 2023
@Exirel Exirel added this to the 8.0.0 milestone Apr 10, 2023
@Exirel Exirel force-pushed the sopel-bind-trigger branch 2 times, most recently from 622dbbe to 8865fb0 Compare April 22, 2023 06:37
@dgw dgw mentioned this pull request May 3, 2023
4 tasks
For ease of use, Sopel provides a SopelWrapper to plugin callable so
they can use ``bot.say(message)`` without having to specify the
destination: the wrapper will look at the trigger's sender and send the
message back to it (be it a channel or a nick).

However, this has the disavantage of having to 1. maintain a separate
class with dunder methods (boo, magic!), and 2. it makes static typing
more complex.

Welcome Python 3.7 and its new built-in module: contextvars. This
wonderful utility module provides a thread-safe and asyncio-safe object
that can have a context specific value.

Sopel now uses this feature to bind itself to:

* a trigger that matches a rule
* and the triggered rule

so now it can be used instead of a SopelWrapper instance.

The biggest changes are on ``call_rule``, which doesn't require anymore
a SopelWrapper instance, and on the tests, in particular on the trigger
factory's wrapped method, which now returns a Sopel instance.

The second side-effect on test is that now it is not possible to bind a
trigger multiple times on a test bot, and tests have to rely on
Sopel.sopel_wrapper context manager (as Sopel.call_rule does), which
requires a Rule object. This is not perfect, albeit manageable.

In the future, it would be nice to have a better testing utility, maybe
a context manager to do someething like:

```
def some_test(mockbot, triggerfactory):
    with triggerfactory.wraps(mockbot, "PRIVMSG #channel Nick :text") as wrapped:
        wrapped.say('something')
```

Wouldn't it be cool?
@Exirel
Copy link
Contributor Author

Exirel commented May 29, 2023

@SnoopJ @dgw I'd love to have your input on the feature itself, i.e. the possibility to get rid of SopelWrapper. The code probably has its flaw, and I definitively want to write more documentation.

But first, I'd really like to know if you are OK with the idea behind this PR. I can vet for contextvar as a pretty safe way to handle that. However, I'm not sure I really like how to handle the state of the reset token itself, and maybe I'd like something easier for tests. Making the tests easier is something that can be achieved with better factories as well, so there are various options here!

Copy link
Contributor

@SnoopJ SnoopJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a lot to say about this WIP except that this is exactly the kind of thing ContextVar is for, so I'm in favor of it, especially if it means killing off the SopelWrapper class.

I asked on IRC if this introduces any concerns with thread safety, and the short version of the ensuing discussion is that this changeset doesn't introduce any new thread safety issues, but it doesn't resolve existing ones either.

sopel/bot.py Outdated Show resolved Hide resolved
Comment on lines +741 to +746
def sopel_wrapper(
self,
trigger: Trigger,
rule: plugin_rules.AbstractRule,
) -> Generator[Sopel, None, None]:
"""Context manager to bind and unbind the bot to a trigger and a rule.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings about the name sopel_wrapper. On one hand, it matches the old SopelWrapper API, but on the other, it seems like there could be a name that better communicates that we're binding to a particular combination of trigger and rule. Is there a Sopel-ese word for that pairing? If so, maybe bind_thatword() would be a good name. My intuition would be bind_event() but that name is probably misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes! I should have added a "TODO: find a good name", because I don't like it either! Something like bind_context or wraps_context would probably be better I guess? Or maybe bind_response? I used sopel_wrapper for exactly the reason you talk about (it matches SopelWrapper), and I also think it doesn't properly communicate what we want to achieve with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other names:

  • wraps_message
  • wraps_trigger_and_rule
  • just wraps (because why not?)

@dgw
Copy link
Member

dgw commented Jun 15, 2023

Optimistically slotting in for 8.1, assuming @Exirel will rework this branch to follow the plan in #2460. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Tweak Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants