bot, rules: fix rate limiting rules without a rate limit #2629
Conversation
Exirel
added
the
Bugfix
There was a problem hiding this comment.
Other than a minor style nit, I have nothing to say, really.
Not the end of the world if you don't get around to git commit --amend && git push -f before I come back to merge this (next week? probably) but that one tiny section of code I commented on would be slightly more readable. 😅
| @@ -594,27 +594,26 @@ def rate_limit_info( | |||
| ) -> tuple[bool, Optional[str]]: | |||
| if trigger.admin or rule.is_unblockable(): | |||
| return False, None | |||
|
|
|||
| nick = trigger.nick | |||
There was a problem hiding this comment.
| nick = trigger.nick | |
| nick = trigger.nick |
Like that?
There was a problem hiding this comment.
Yes, like that. I guess it allows you to "put back the deleted line" if you select only the "added" line and edit manually.
All this time and the suggestion UX is still "meh" at best. 🫤
There was a problem hiding this comment.
Well, I suggested as much as I could relating to the unnecessary *_metrics assignments. GitHub won't let me add a line note to the metrics = global_metrics line, so sorry, you're on your own.
This will not fix errors in mypy 1.12, though. What this code does with channel is incompatible with the AbstractRule interface.
| is_channel = trigger.sender and not trigger.sender.is_nick() | ||
| channel = trigger.sender if is_channel else None | ||
|
|
||
| at_time = trigger.time | ||
|
|
||
| user_metrics = rule.get_user_metrics(trigger.nick) | ||
| user_metrics = rule.get_user_metrics(nick) | ||
| channel_metrics = rule.get_channel_metrics(channel) | ||
| global_metrics = rule.get_global_metrics() |
There was a problem hiding this comment.
With mypy 1.12.0 (released today), channel_metrics = rule.get_channel_metrics(channel) causes a type-check error at R602 in this patch's version of the file:
sopel/bot.py:604: error: Argument 1 to "get_channel_metrics" of "AbstractRule" has incompatible type "Any | None"; expected "Identifier" [arg-type]
Since the revised code doesn't use the RuleMetrics objects in this section any more, the block of *_metrics assignments here can be removed.
That won't fix the other problem, though, which is that both rule.is_channel_rate_limited() and rule.get_channel_metrics() expect Identifier and do not accept None.
| user_metrics = rule.get_user_metrics(nick) | ||
| channel_metrics = rule.get_channel_metrics(channel) | ||
| global_metrics = rule.get_global_metrics() | ||
|
|
There was a problem hiding this comment.
| user_metrics = rule.get_user_metrics(nick) | |
| channel_metrics = rule.get_channel_metrics(channel) | |
| global_metrics = rule.get_global_metrics() |
| channel_metrics = rule.get_channel_metrics(channel) | ||
| global_metrics = rule.get_global_metrics() | ||
|
|
||
| if user_metrics.is_limited(at_time - rule.user_rate_limit): | ||
| at_time = trigger.time | ||
| if rule.is_user_rate_limited(trigger.nick, at_time): | ||
| template = rule.user_rate_template | ||
| rate_limit_type = "user" | ||
| rate_limit = rule.user_rate_limit | ||
| metrics = user_metrics |
There was a problem hiding this comment.
| metrics = user_metrics | |
| metrics = rule.get_user_metrics(nick) |
| channel_metrics = rule.get_channel_metrics(channel) | ||
| global_metrics = rule.get_global_metrics() | ||
|
|
||
| if user_metrics.is_limited(at_time - rule.user_rate_limit): | ||
| at_time = trigger.time | ||
| if rule.is_user_rate_limited(trigger.nick, at_time): |
There was a problem hiding this comment.
| if rule.is_user_rate_limited(trigger.nick, at_time): | |
| if rule.is_user_rate_limited(nick, at_time): |
Unrelated to the refactor I'm suggesting re: metrics objects; this just doesn't use the local variable for some reason?
| template = rule.user_rate_template | ||
| rate_limit_type = "user" | ||
| rate_limit = rule.user_rate_limit | ||
| metrics = user_metrics | ||
| elif is_channel and channel_metrics.is_limited(at_time - rule.channel_rate_limit): | ||
| elif is_channel and rule.is_channel_rate_limited(channel, at_time): | ||
| template = rule.channel_rate_template | ||
| rate_limit_type = "channel" | ||
| rate_limit = rule.channel_rate_limit | ||
| metrics = channel_metrics |
There was a problem hiding this comment.
| metrics = channel_metrics | |
| metrics = rule.get_channel_metrics(channel) |
Description
Fix #2627 by checking if the rate limit is actually set (equal or less than 0s).
I decided to make the
at_timeparameter mandatory, as it's all internal stuffs and it makes it clearer what time we are comparing it to. That's how I figured out that it was about the repeating call to the same rule for a single trigger: it would check against the same trigger object, but compare it to the latest call tonow(). That's why the bug was visible onurlrules, and it affectedfindrules as well.Checklist
make qa(runsmake lintandmake test)