sopel icon indicating copy to clipboard operation
sopel copied to clipboard

Use Python's contextvars module to remove SopelWrapper

Open Exirel opened this issue 1 year ago • 2 comments

Requested Feature

This is a long term plan: the end goal is to remove SopelWrapper by using contextvars in the Sopel class itself. The main issue is that removing the SopelWrapper may break the following cases:

  • using isinstance will break
  • type annotations using SopelWrapper will obviously fail, and mypy will complain
  • tests may need some adaptation to deal with a context specific bot instance

All these are perfectly acceptable for a major release, but shouldn't be introduced in a minor release. We should also ensure a period of deprecation, with warning in the documentation, and if possible in the code itself.

After a conversation on IRC, a rough plan is:

  • [x] Sopel 8.0: warn in the documentation/release note/somewhere else that we are deprecating SopelWrapper #2521 (target version for removal unspecified for now, but probably 9.0)
  • [ ] Sopel 8.1: #2526
  • [ ] Sopel 8.1: improve testing tools so it is much easier to test with a "context sensitive instance" to ease future breaking change
  • [ ] Sopel 8.1: replace SopelWrapper by Sopel in the code, and add code deprecation warning to prevent its future usage
  • [ ] Sopel 8.1: extensively document how to test with Sopel instead of SopelWrapper
  • [ ] Sopel 9.0: #2527

Problems Solved

Today, we use SopelWrapper to wrap an instance of Sopel and use it in plugin callable (the bot argument), and as a result:

  • we have an extra class to take care of
  • the documentation is split between 3 classes (the irc abstract class, Sopel, and SopelWrapper)
  • it uses magic method and magic behavior of Python related to attribute management, and we don't really need that
  • whenever we add/remove a fonction on Sopel we have to consider how it will or can be used with SopelWrapper, requiring more code, more tests, maybe more documentation, etc.
  • function signature need to know if they want a SopelWrapper object, a Sopel object, or both, which can be confusing

Notes

PR #2443 started the conversation around contextvars and removing SopelWrapper, but making such a drastic change in Sopel 8 is too much, hence this issue with a proper plan with a target version as Sopel 9.0.

Exirel avatar Jun 02 '23 15:06 Exirel

Summarizing short discussion from IRC: 8.0 is very long in the tooth now, it seems like we should at-most include a note in the docs about our intent to get rid of SopelWrapper Eventually™ for 8.0 with no code changes, and we can figure out the rest of the details when 8.0 is out the door

@dgw suggested 9.0 for release of the initial implementation of a contextvars replacement, which I think would push the removal of SopelWrapper to 10.0, but we can discuss more as we go.

SnoopJ avatar Jul 03 '23 15:07 SnoopJ

@dgw suggested 9.0 for release of the initial implementation of a contextvars replacement, which I think would push the removal of SopelWrapper to 10.0, but we can discuss more as we go.

Thought I clarified in a follow-up line that I meant the switchover would happen in 9.0, with the implementation first shipping in 8.1, but maybe that got lost. x)

dgw avatar Jul 03 '23 15:07 dgw