Open dgw opened 5 years ago
Started digging in the code regarding this since I am actively interested in #1660 . Trying to wrap my head around what happens here for 'intents' currently.
Warning: This may turn into a rant/rambling.
Note: testing was done on UnrealIRCd-4.2.2
**Obviously, whatever direction this goes, we may want to retain some backwards compatibility for older ircd servers. I'm fairly certain that unrealircd is a participant in ircv3, so I'm wondering what other tag ACTION would fall under besides 'intent'.
looks like Message Tags is somewhat already there as of 4.1.0?
https://github.com/sopel-irc/sopel/blob/master/NEWS#L1209
Looks like this is where we parse the tags into a usable format: https://github.com/sopel-irc/sopel/blob/master/sopel/trigger.py#L34-L43
At a glance, the code looks like it should be making a reference of all the tags a message contains even if we aren't looking for it. Curious what tags are possible here.
I wrote a test module just to spit out what trigger.tags
as well as trigger.raw
contains:
<~deathbybandaid> .tap
<testbot> {'time': '2019-09-18T14:58:54.520Z'}
<testbot> @time=2019-09-18T14:58:54.520Z :deathbybandaid!deathbyban@irc.redacted.net PRIVMSG #deathbybandaid :.tap
* ~deathbybandaid tests things
<testbot> {'time': '2019-09-18T14:59:09.100Z', 'intent': 'ACTION'}
<testbot> @time=2019-09-18T14:59:09.100Z :deathbybandaid!deathbyban@irc.redacted.net PRIVMSG #deathbybandaid :ACTION tests things
Okay, so I see what the code does, but I am also confused at the raw line. This made me glance at intent_regex
specified in the function. I'm not seeing the \x01
to be matched, but that may be because those bytes didn't go into str()
. But even the ACTION
part of the message isn't contained in the same section as time
which is considered a normal "tag".
Considering Sopel creates the dict key of 'intents': https://github.com/sopel-irc/sopel/blob/master/sopel/trigger.py#L102
It seems like it should be fairly trivial to migrate the term "intents" into something like a boolean for is_action
The trouble is, the only "tag" that I'm seeing is time
.
This led me to the unrealircd forums regarding unreal5 alpha:
Enhancements:
Support for server generated message tags, which allows us to communicate additional information in protocol messages such as in JOIN and PRIVMSG. Currently implemented and permitted message tags are:
* account: communicate the services account that a user uses
* msgid: assign an unique message id to each message
* time: assign a time label to each message
So it looks like maybe an ACTION tag isn't quite there yet. However, I could see "msgid" being useful.
I then googled until I got to this page: https://ircv3.net/registry#tags
But I still don't see anything standardized for ACTION.
tldr: Nuke intents
, and implement is_action
,,,, message tags seems to already be in the code?
after some conversation with the unrealircd guys,,, it was also discovered that my tests had some issue.
since my test bot, and myself are behind ZNC, I am getting some tags attached by ZNC that aren't coming from my unrealircd,,, so that's interesting
There isn't anything standardized for ACTION as far as I know, just the CTCP type we all know and love. It was also the only defined intent that I was ever aware of, and Sopel was (is still) emulating it. I don't believe ACTION as an intent (rather than CTCP) ever made it into implementations.
Yes, there are more tags coming via additional specs. They're not particularly relevant to this issue, but they're not completely irrelevant.
so I may be reading this wrong,,,
via https://ircv3.net/specs/extensions/message-tags.html
"""" In this example
The server sends these messages:
S: :nick!user@example.com PRIVMSG #channel :https://example.com/a-news-story
S: @+icon=https://example.com/favicon.png :url_bot!bot@example.com PRIVMSG #channel :Example.com: A News Story
"""
does this mean that sopel could potentially send tags as well as receive?
Sopel definitely could have a way to send tags, but let's leave that as a separate feature discussion. It doesn't affect this deprecation at all, which only matters for incoming messages.
Anyway, the reason I came here (and then got distracted by the most recent comment) was:
We can probably straight-up replace the @module.intent
decorator with @module.ctcp
. The only place I could find it used was in version.py
, where it catches CTCP requests for VERSION, SOURCE, PING, and TIME (note that only one of those actually belongs in the "version" plugin, maybe two).
The deprecation cycle for that is easy—we add ctcp
in 7.x and remove intent
in 8.0. What's not so easy is coming up with new names/locations for the internal stuff: func.intents
on callables, trigger.tags['intent']
, etc.
Here's the thing: It's already October. We should aim to get 7.0 out in December at the latest. I'd be perfectly happy doing only the bare minimum for 7.0 (adding the @module.ctcp
decorator, just to have it in the API, and marking it as @module.intent
's replacement), and leaving everything else for 7.1. Or we can push this whole thing off to 7.1, despite what I said in the OP. There's just still a lot to do in three months, and the intents API features are "good enough" to last until the next release cycle even if the spec they were based on is dead and gone.
I think we can add a @plugin.ctcp
in 7.1 and see what we do with @plugin.intent
later in 8.x. It's not that of a big deal and I'm not really worried to have to support that until 9.x.
A revision of that idea: What if we add @plugin.ctcp
and drop @plugin.intent
, since (once again) we have the perfect excuse in the form of deprecating sopel.module
? We're already leaving new stuff out of module
; why not omit old stuff from plugin
too?
So in #1975 I replaced sopel.plugin.intent
by sopel.plugin.ctcp
, and added a new feature (to use ctcp
directly without argument as a decorator). I updated the doc.
I think we can pin this issue to Sopel 8 now, and see what we can do to change how Sopel deals with CTCP commands internally.
@Exirel Added a task list to the OP as follow-up to review/discussion in #1975 and IRC. I think the Trigger
one is the only thing that needs doing for 7.1.
I ended up updating #1975 to add the trigger.ctcp
property, I think that's a fairly small change that can be done right now.
Noted the updated status, and added an optional item for later (8.x) to flag use of the deprecated trigger.tags['intent']
key if we so choose. (It'd be useful, but IMO not enough to warrant #1809 levels of hackery if that's what it would take.)
trigger.tags['intent']
field.And I honestly don't know how to deal with that part. How do we know the difference between "I want to use intent as CTCP" vs "I want to use intent as any other tags"? Beside, if any plugin out there try to use trigger.tags['intent']
directly, they are already doing something wrong anyway, given that there is trigger.intent
/ trigger.ctcp
now.
So I say: it's not worth the effort.
The point is, there won't be any intent
tag. Any access of that key is by definition trying to use obsolete behavior, so just emit a deprecation warning?
so just emit a deprecation warning?
I left this on the side, to see if I could think of anything good with some times. And yet, that's the problem: remember when @half-duplex tried to do that for SopelMemory
and the URL callback's keys? That didn't go well. As for tags
, it's not even a SopelMemory
object, so I would have to create a whole new class just to allow deprecation warning on something that should not be used anyway (even as of Sopel 7.x).
More documentation, that I can do. More tests, that I can do. Emit a warning when someone tries to do trigger.tags['intent']
... that's quite complicated, and I still think it's not worth it.
Removed a deprecation warning for trigger.tags['intent']
from the task list, per the reasons discussed between myself and @Exirel (very leisurely) last year. All that's left is for 9.0; re-milestoned.
Task summary
@intent
decorator [7.1, #1975]@intent
decorator [9.0, whensopel.module
goes away]@ctcp
decorator [7.1, #1975]Trigger
objects need actcp
attribute instead of parsing CTCPs intoTrigger.tags['intent']
[7.1, #1975]@property
that just returns the value ofself.tags['intent']
; what's important is providing the future-compatible API ASAP.[ ] Some indication to plugins that accessNAK (comment w/reasoning)trigger.tags['intent']
would be good [8.0]Rule
's interface and subclasses define amatch_intent()
method that needs replacing [8.0, #2253]intents
attribute (added by the@ctcp
decorator from #1975) replaced [8.0, #2253]intents
as the internal attribute name, since@module.intent
simply wraps the@plugin.ctcp
implementation.Original issue description follows.
The
intents
feature is long dead, having been pulled from IRCv3 almost three years ago (ircv3/ircv3-specifications#271). This affects Sopel in a few ways.First, we have a
@module.intent
decorator that is now obsolete, named for a dead specification that nobody is going to implement.Second, Sopel internally handles CTCP ACTIONs (the only use case I was ever aware of for intents) by stripping the CTCP bytes and adding the "ACTION" intent to the trigger instead. That's why plugin functions wanting to act only upon ACTION messages must use
@module.intent('ACTION')
.Third, obviously the
intents
attribute added to Sopel callables with intent filters is also obsolete.In short, there's a fair bit of restructuring to do here. A quick chat with the IRCv3 folks on freenode tells me that CTCP ACTIONs themselves aren't going anywhere, so we should be fine designing an API around those as they currently exist. Maybe we add
@module.action
, like@module.echo
, and kill off regular@module.commands
-decorated callables receiving ACTION messages at all. (That would take care of an oddity that exists in current Sopel 6.x, wherein one can do/me .command
and Sopel will act on it just like a regular message, which can be unexpected.)That same chat on IRC also told me that the direct replacement for intents is message-tags. We don't support those at all, at least not in a way that plugins can use.
I'd like to get this done for Sopel 7.0, but only deprecating
@module.intent
and replacing it with something else is truly important. Opening message-tags up in the plugin API can wait until 7.1+.