sopel
sopel copied to clipboard
Use Python's contextvars module to remove SopelWrapper
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, andmypy
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
bySopel
in the code, and add code deprecation warning to prevent its future usage - [ ] Sopel 8.1: extensively document how to test with
Sopel
instead ofSopelWrapper
- [ ] 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
, andSopelWrapper
) - 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 withSopelWrapper
, requiring more code, more tests, maybe more documentation, etc. - function signature need to know if they want a
SopelWrapper
object, aSopel
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.
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.
@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)