Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bot, rules: fix rate limiting rules without a rate limit #2629

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Oct 12, 2024

Description

Fix #2627 by checking if the rate limit is actually set (equal or less than 0s).

I decided to make the at_time parameter 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 to now(). That's why the bug was visible on url rules, and it affected find rules as well.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make lint and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Oct 12, 2024
@Exirel Exirel added this to the 8.0.1 milestone Oct 12, 2024
@Exirel Exirel requested a review from dgw October 12, 2024 13:51
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Show resolved Hide resolved
@dgw dgw mentioned this pull request Oct 14, 2024
4 tasks
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
sopel/bot.py Outdated Show resolved Hide resolved
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, it looks so much cleaner now without the extra *_metrics locals. 😁

Good to squash

Co-authored-by: dgw <dgw@technobabbl.es>
@Exirel
Copy link
Contributor Author

Exirel commented Oct 18, 2024

Good to squash

Done.

@dgw dgw merged commit 08a0fce into sopel-irc:master Oct 26, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rate limiting can be incorrectly applied to callables with no limit set
2 participants