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

coretasks: update event priorities from high to medium #2018

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Jan 19, 2021

Description

Fix #2017 and add a new chapter for advanced tips and tricks.

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 this to the 7.1.0 milestone Jan 19, 2021
@Exirel Exirel requested a review from dgw January 19, 2021 23:21
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I might have to go over this again from a computer, after this first review round is addressed, to catch any non-obvious typos that don't jump out at me on GitHub's Android app, but just in general: Why remove the priorities from all those coretasks handlers instead of using "medium", following the Tao of Python's advice that "[e]xplicit is better than implicit"?

docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
sopel/coretasks.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jan 26, 2021

All fixed!

@Exirel Exirel requested a review from dgw January 26, 2021 23:27
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I left coretasks.py itself un-marked as "Viewed" because I'm still curious about removing @plugin.priority from the relevant callables instead of explicitly specifying @plugin.priority('medium'). That's my main concern; the rest of this review is just typos and phrasing tweaks.

docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jan 30, 2021

explicitly specifying @plugin.priority('medium')

I can do that, no problem.

docs/source/plugin/advanced.rst Outdated Show resolved Hide resolved
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

@Exirel I fixed my own stupid typo, so feel free to squash stuff down when ready.

Exirel and others added 2 commits February 7, 2021 15:18
* events are now at default priority (medium)
* events are not threaded anymore
* events are now unblockable

This will allow other plugin to act before (priority=high) or before
(priority=low) the core plugin handles them.

One event is still on low priority as it will send a QUIT message.
Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel
Copy link
Contributor Author

Exirel commented Feb 7, 2021

Done.

@dgw dgw merged commit e4a8c12 into sopel-irc:master Feb 19, 2021
@Exirel Exirel deleted the coretasks-event-priority branch May 1, 2021 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition when plugins want to access bot state after PART/QUIT
2 participants