sopel icon indicating copy to clipboard operation
sopel copied to clipboard

plugin: shift to `Enum` for `priority`

Open dgw opened this issue 1 year ago • 4 comments

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:

  • [x] Type-checker alerts in 8.0 (#2555)
  • [ ] Add an Enum type and accept both that and regular str in 8.1
  • [ ] Add a warning using lifecycle.deprecated() if the decorator is used with a plain str in 8.1
  • [ ] Remove support for the plain str argument in 9.0

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.

dgw avatar Nov 11 '23 21:11 dgw

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"

half-duplex avatar Nov 11 '23 22:11 half-duplex

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. 😑

dgw avatar Nov 11 '23 22:11 dgw

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.

SnoopJ avatar Nov 11 '23 22:11 SnoopJ

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.

dgw avatar Nov 11 '23 22:11 dgw