bot, coretasks, irc, plugin, plugins: new capability negotiation system #2341
Conversation
Exirel
added
Feature
Work In Progress
Breaking Change
Exirel
force-pushed
the
plugin-cap-decorator
branch
2 times, most recently
from
9f8b81b
to
f3ff776
Compare
|
So, it's far from it, but it's getting there:
We are reaching slowly, but surely, the 60% coverage rate! |
Exirel
force-pushed
the
plugin-cap-decorator
branch
from
f3ff776
to
a6fc551
Compare
There was a problem hiding this comment.
OK, I'm going to stop this round before I get absolutely lost in test/. There's a lot of new stuff in there, but as long as the suite passes there's no reason to spend time looking in there yet.
Some big "what if" and "why not" questions from me. I'll probably be excoriated for even thinking some of them—and with good reason 😁—but I hope that overall, this collection of feedback will be useful in polishing the approach here.
I took the time to read everything, and I think I tried to reply to most of the important things. I think most of the questions are about a very specific issue with the protocol's specification: what does it mean to validate a request? Is a request that disable something not available is valid, or not valid? single cap request vs multi cap requestIt is very easy to deal with single-cap request: it either works, or it doesn't. The problems come only when there are multi-cap requests:
From reading the spec, it is entirely possible for a server to NAK the request That's also why I don't want Sopel to send one single callback anti-patternWhile working on that, I discovered some sort of anti-pattern: registering a handler for a CAP REQ that changes the state of the bot (or its memory), or load something. This is, except for one exception, a bad idea. The proper way to handle when a capability is enabled or disabled is to use the method It is especially true for capability that are not available, so the bot should not request them, and so it won't received either an ACK nor a NAK. In that case, the callback won't be called. The good reasons I've seen so far are:
I think we should agree on that, or find a consensus about that, and then document it (probably in an "advanced" chapter of the documentation). |
Being able to interact directly with the IRC protocol, as in the case of SASL, is the primary reason that CAP requests need a way to register callbacks, and specifically a way to delay the end of CAP negotiation until the request has done whatever else it needs to do. That's a core function, of course, and so it needs to be supported somehow. All of this capability-negotiation stuff is already way above the pay grade of most plugin authors, who will never need to know how to use it. It's fine for an "advanced" chapter, likely one with judicious use of There's also no reason not to mark the whole capability request / manager system as provisional, "subject to rapid changes between versions" and all that jazz like we say about the internal plugin machinery, so we have the freedom to iterate on it if necessary without having to think about deprecations. (If including provisional modules in a few releases before marking them as stable is good enough for CPython, it's good enough for us!) |
And it is! So we are good on that front. I've re-read all the comments, and now I think I'm confident with my current design. I have some improvements in mind, such as:
Given your blessing to make this as provisional, I may suggest to just go with it for now, and see how we can improve that further. For instance, even thought I'm against, working around the "this CAP is not available so -CAP should not be a problem" thing is something that we may want to work on. We could also change the way to declare CAP REQ in a plugin code. I'll try to fix all the spelling & docstring issues today. |
There was a problem hiding this comment.
Scary line note count, but it's mostly typo fixes.
You were right when you said on IRC that this was waiting on me taking another look. I think for my part, when looking for things that needed attention in the PR list, I have been skipping anything that's still marked as a draft. It's probably time to Ready this one after this set of comments, isn't it?
|
Everything that was typo-related has been fixed. Let me know if you want me to squash this before doing another review, other than that, it's ready on my end. (and so so so sorry for the long delay, between late November and today, a lot happened on my side and I wasn't able to make this PR to move forward as I intended) |
Exirel
force-pushed
the
plugin-cap-decorator
branch
from
6ba0803
to
0f7bab9
Compare
There was a problem hiding this comment.
Submitting my second-opinion now, without having gone through the test suite with a fine-toothed comb.
Based on the number of my remarks that ended up being nitpicks and docs, I say this looks good to me. Some parts are a little tricky to follow but overall I 'get' it.
There was a problem hiding this comment.
There are a couple of nits that can be handled during squash, but we should make a decision about one apparently unresolved thread that led into discussion about CAP REQs that are too long to be acknowledged in one line. It's fine to say that's for a later patch, but in that case let's have a follow-up issue about it before merging. 😸
|
Oof. Done with the third? (I didn't count) round. I think it's the third, or maybe the fourth... Anyway, I added the limitation on the Permission to rebase & squash requested! 😁 |
There was a problem hiding this comment.
Just style & typo nits. Any style mishaps I've missed at this point will be allowed to live to see the day when a relevant flake8 plugin is added and flags them for correction, if ever.
I'm still worried a bit about the possibility of (as @SnoopJ called it) "a particularly antisocial capability" named with a leading hyphen that would be stripped in plugins.capabilities.Manager.request_available(), but that's a concern that needs to be run by the IRCv3 folks. If it's a possibility, a small patch later can try to account for it. No need to hold this any longer on account of such a wildly unlikely case.
I have honestly never think that it would be possible. There is nothing that technically prevent that from happening for standard capabilities, however they should start as a draft first, so people will see the problem quickly. As for vendor capabilities:
And a valid DNS domain name MUST NOT start with an hyphen, and in case there is a non-ASCII character in there:
So I think the spec covers the problem by not allowing anyone to do that in practice. Also, PR is ready again! |
The `sopel.irc.capabilities` submodule aims to implement part of the IRCv3 specification for Capability Negotiation. Its goal is to store what the bot knows about capabilities: what is available and what is enabled. This submodule doesn't manage capability requests. Co-authored-by: dgw <[email protected]> Co-authored-by: James Gerity <[email protected]>
The `capability` decorator, and the `CapabilityNegotiation` enum allow a plugin to request capabilities. This will replace the current system with the CapReq class and the irc.AbstractBot.cap_req method that nobody uses because it's way too complex and convoluted (also it doesn't do what it says it does, or not properly). Collecting capability requests and handling them will be part of another commit. Co-authored-by: dgw <[email protected]>
Third parts of the new Capability Negotiation implementation: having a way to track capability requests and their resolution. Next stop: using these three parts together into the bot and coretasks. Co-authored-by: dgw <[email protected]> Co-authored-by: James Gerity <[email protected]>
This is the last part of the rework: actually use all the right tools for the job, and deprecate/remove the old wonky ones. Namely: * move `bot.cap_req` from `AbstractBot` to `Sopel`, and deprecate it * replace various attributes by properties from the `irc.capabilities` manager * add the cap request manager (`bot.cap_requests`), and use it to know more about the capability requests, and to register requests * deprecate `irc.utils.CapReq` * completely rework `sopel.coretasks`'s management of capability negotiation, and also SASL authentication * add so **many** tests for coretasks (CAP & SASL related) Co-authored-by: dgw <[email protected]>
…ed-join capabilities
Co-authored-by: dgw <[email protected]>
Co-authored-by: dgw <[email protected]> Co-authored-by: James Gerity <[email protected]>
Co-authored-by: dgw <[email protected]>
Exirel
force-pushed
the
plugin-cap-decorator
branch
from
94ab383
to
7ad3c59
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def test_capability_handler_define_once(): | ||
| @plugin.capability('away-notify') | ||
| def handler(name, bot, acknowledged): | ||
| ... |
Check notice
Code scanning / CodeQL
Statement has no effect
There was a problem hiding this comment.
Unfortunately for @Exirel I went through the whole thing* again and found a few more nitpicks—but more importantly, I found an example in one docstring that seems to need clarification.
* — Except for tests. I didn't reread most of the test/ files.
Co-authored-by: dgw <[email protected]>
Both bot attributes have been deprecated in 8.0, add warnings in 8.1, to be removed in 9.0: * `bot.enabled_capabilities` * `bot.server_capabilities` Co-authored-by: dgw <[email protected]>
Description
Fix #972 and should fix #974 as well.
I rewrote the entire capability negotiation feature.
I haven't tested it live properly, and I want to add so much more tests, this will have to wait.
Don't review the docstring yet or you are going to have a bad time, as I'm going to rebase/squash this multiple times until I'm happy with the result.
This PR exists so anyone can take a chance to test it live on their own setup.
Checklist
make qa(runsmake qualityandmake test)