You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A big goal of the Rule System rewrite (major x-refs: #1873, #1904; see also #1816) was to make using as many different decorators together as we could, possible. Especially with Sopel 7.1, I thought (hoped?) that we were finally past the days of needing a "main" callable to handle e.g. URLs, and a "wrapper" callable to handle other rules. As I start updating plugins like sopel-github to what I thought would be the 7.1 style, though, I find that that's not true. A callable with e.g. @url and @find will show in debug logs that a URLCallback was registered, but the FindRule is never registered. Comment out the @url and the FindRule works.
The code below (from loader.is_triggerable()) forbids registering regular rules (Rule, FindRule, SearchRule, etc.) or commands for callables that are decorated with @url or @url_lazy, and appears to be the culprit:
Having found the root cause, I am probably done with this for today (lots going on IRL this week), but I wanted to make sure what I figured out so far isn't lost to the void of IRC logs. Short version: I think the obvious solution (delete those two URL-related forbidden_attrs) is likely to cause more problems later. (Why? Keep reading. 🙂)
What I don't have the mental energy for today is continuing to trace the history of that change beyond #1708 (specifically 7347b30, which talks about 'jobs' only, i.e. @interval, not URLs), and planning (or implementing) the solution. I suspect these forbidden_attrs are also related to #1709, a follow-up to the previous patch that relies on is_triggerable() to gatekeep the addition of default attributes used by the loader and rule system.
At minimum, taking those attrs out of is_triggerable() would mean adjusting logic elsewhere to compensate. More importantly, there's still some weird special handling (e.g. here in coretasks) that deals with URLs separately from the regular dispatch pathway (via on_message() or on_message_sent() in irc). If we're going to make URLCallback part of the Rule System, the same logic that dispatches every other Rule type should also handle URLs, IMO. A discussion about how to do that (or why I'm wrong) would be very useful as we reach the home stretch of work on 8.0.
The text was updated successfully, but these errors were encountered:
dgw
added
Bug
Things to squish; generally used for issues
Needs Triage
Issues that need to be reviewed and categorized
labels
Nov 10, 2022
A big goal of the Rule System rewrite (major x-refs: #1873, #1904; see also #1816) was to make using as many different decorators together as we could, possible. Especially with Sopel 7.1, I thought (hoped?) that we were finally past the days of needing a "main" callable to handle e.g. URLs, and a "wrapper" callable to handle other rules. As I start updating plugins like
sopel-github
to what I thought would be the 7.1 style, though, I find that that's not true. A callable with e.g.@url
and@find
will show in debug logs that aURLCallback
was registered, but theFindRule
is never registered. Comment out the@url
and theFindRule
works.The code below (from
loader.is_triggerable()
) forbids registering regular rules (Rule
,FindRule
,SearchRule
, etc.) or commands for callables that are decorated with@url
or@url_lazy
, and appears to be the culprit:sopel/sopel/loader.py
Lines 206 to 211 in 81b5e11
Having found the root cause, I am probably done with this for today (lots going on IRL this week), but I wanted to make sure what I figured out so far isn't lost to the void of IRC logs. Short version: I think the obvious solution (delete those two URL-related
forbidden_attrs
) is likely to cause more problems later. (Why? Keep reading. 🙂)What I don't have the mental energy for today is continuing to trace the history of that change beyond #1708 (specifically 7347b30, which talks about 'jobs' only, i.e.
@interval
, not URLs), and planning (or implementing) the solution. I suspect theseforbidden_attrs
are also related to #1709, a follow-up to the previous patch that relies onis_triggerable()
to gatekeep the addition of default attributes used by the loader and rule system.At minimum, taking those attrs out of
is_triggerable()
would mean adjusting logic elsewhere to compensate. More importantly, there's still some weird special handling (e.g. here incoretasks
) that deals with URLs separately from the regulardispatch
pathway (viaon_message()
oron_message_sent()
inirc
). If we're going to makeURLCallback
part of the Rule System, the same logic that dispatches every other Rule type should also handle URLs, IMO. A discussion about how to do that (or why I'm wrong) would be very useful as we reach the home stretch of work on 8.0.The text was updated successfully, but these errors were encountered: