sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
951 stars 405 forks source link

plugin: shift to `Enum` for `priority` #2557

Open dgw opened 7 months ago

dgw commented 7 months ago

We've taken a baby step toward this with #2555, which will flag unrecognized arguments at the type-checker level (for anyone who actually bothers running a type-check on their plugins… and most people probably don't bother, myself historically included).

But in the long term, it'd be better to have enumerated values for something like this, the same as we have for formatting colors (#2122), events (#2127), and privilege levels (#2540). It will give us a ton more freedom in the future to improve the priority system.*

The full rollout process I had in mind is something like this:

These planned steps are subject to revision based on discussion, real-world concerns expressed by users, and all the other usual factors.

I'll start this issue out in the first milestone with an action item under the proposed plan, which is 8.1.0.


* — Priorities will most likely change from str to int somewhere in this process, also to help with flexibility. Using an Enum instead of literal str values will help us shield plugin developers from that transition in the long run.

half-duplex commented 7 months ago

I don't like removing support for str in a minor release, it should be deprecated in an 8.* and removed in 9.0 (or I guess 10.0). Semver: "MINOR version when you add functionality in a backward compatible manner"

dgw commented 7 months ago

Updated to shift the last two steps back a version (warning also in 8.1 now; removal in 9.0).

I want this str arg gone. 😑

SnoopJ commented 7 months ago

I'm a -1 on removing str at all. It makes sense to me to define the enumeration as class Priority(Enum, str) (or with StrEnum in 2026 once 3.11 is the oldest extant version of Python), but it seems like strictly a degradation to stop accepting the str form of the parameter, unless we are very committed to a numeric priority, which I will admit is quite a bit more flexible.

dgw commented 7 months ago

unless we are very committed to a numeric priority, which I will admit is quite a bit more flexible.

I'm very committed to making priorities more flexible, personally. Can't speak for the other maintainers, but I've never liked having literal strings here. Switching to an Enum here doesn't make that much sense to me vs. just annotating the function with valid literals (#2555) if we aren't also planning to change what the Enum values represent at some point.