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

Use Python's contextvars module to remove SopelWrapper #2460

Open
1 of 4 tasks
Exirel opened this issue Jun 2, 2023 · 3 comments
Open
1 of 4 tasks

Use Python's contextvars module to remove SopelWrapper #2460

Exirel opened this issue Jun 2, 2023 · 3 comments
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Documentation Feature
Milestone

Comments

@Exirel
Copy link
Contributor

Exirel commented Jun 2, 2023

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:

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 Exirel added Feature Needs Triage Issues that need to be reviewed and categorized labels Jun 2, 2023
@Exirel Exirel added this to the 8.0.0 milestone Jun 2, 2023
@dgw dgw mentioned this issue Jun 15, 2023
4 tasks
@SnoopJ SnoopJ added Documentation Breaking Change Stuff that probably should be mentioned in a migration guide labels Jul 3, 2023
@SnoopJ
Copy link
Contributor

SnoopJ commented Jul 3, 2023

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

dgw commented Jul 3, 2023

@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)

@Exirel Exirel removed the Needs Triage Issues that need to be reviewed and categorized label Jul 3, 2023
@dgw dgw mentioned this issue Aug 17, 2023
4 tasks
@SnoopJ SnoopJ modified the milestones: 8.0.0, 8.1.0 Oct 24, 2023
@dgw dgw modified the milestones: 8.1.0, 9.0.0 Oct 21, 2024
@dgw
Copy link
Member

dgw commented Oct 21, 2024

With the Issues preview, newly added sub-issues of this one can live in the intermediate milestone(s) like 8.1.0. This one can move to 9.0.0 so it doesn't clutter the 8.1 list.

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 Documentation Feature
Projects
None yet
Development

No branches or pull requests

3 participants