tools, plugins: remove tools.jobs and move Job to plugins.jobs#2725
tools, plugins: remove tools.jobs and move Job to plugins.jobs#2725Exirel wants to merge 3 commits intosopel-irc:masterfrom
Conversation
Exirel
added
Breaking Change
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dgw
left a comment
dgw
left a comment
There was a problem hiding this comment.
I have checked over perhaps 90% of this, mostly skimming over some of the trivial test changes.
Since we have the tests, most of the comments are expectedly related to docs.
| def unregister_jobs(self, jobs: Iterable) -> None: | ||
| for job in jobs: | ||
| self._scheduler.remove_callable_job(job) |
There was a problem hiding this comment.
Aaaa, removing something with no deprecation warning in a minor version 🙀
But it's not advertised in the API docs (no docstring), so… it's probably fine? 😅
| handler = plugin.interval(wait_interval)( | ||
| plugin.label('throttle_join')( | ||
| _join_event_processing | ||
| ) | ||
| ) | ||
| bot.scheduler.register(job) | ||
| handler.plugin_name = 'coretasks' | ||
| bot.register_jobs([handler]) |
There was a problem hiding this comment.
If I'm honest, this feels a bit too "voodoo" for long-term use, even though conceptually it's just "using decorators without @ syntactic sugar". Can we make a better interface?
There was a problem hiding this comment.
Hm... on one hand, yes it's just how you can use decorators. And the other hand, the PluginJob new class is not public yet, and I'm not sure it's ready to be, even tho it's probably a very simple interface to use.
So... it's kinda hard to say what's best here.
I also want to point out that, until the PluginJob/PluginCallable rework, you had to use clean_callable to make sure it was possible to register the job: there is now one less step to do so!
| The ``manager`` parameter can be any kind of object; usually it's an | ||
| instance of :class:`sopel.bot.Sopel`. |
There was a problem hiding this comment.
Is this still true given type hints for execute() enforcing only Sopel? (Line 487)
There was a problem hiding this comment.
Ooooooh good catch. I need to see what I can do with that. Since it's now in sopel.plugins.jobs... I'd argue it should always require Sopel as the manager.
Co-authored-by: dgw <dgw@technobabbl.es>
3 participants
Description
Fix #2476.
As per the discussion in the issue, I removed
sopel.tools.joband moved the Job class tosopel.plugins.jobs. While I was there, I added a couple of tests (which required small adjustments here and there), and made sure to addversionaddedorversionchangedin the docstrings.I may want to add more tests in the future, but testing the time related multithreading thing is annoying to automate.
Checklist
make qa(runsmake lintandmake test)