-
-
Notifications
You must be signed in to change notification settings - Fork 409
bot, loader, plugin, rules: add NOTICE message to rate limits #2290
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
Conversation
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't dived into the tests yet (there are a lot of them), but there might be enough to discuss already without them.
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I realize some of this could fall in "future PR" territory. One of us can make a tracking issue(s) once we agree on what's out of scope.
|
I've modified Also now there are conflicts... I didn't look at them yet. |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge conflicts are from a bit of CTCP stuff and some type-hint tweaks, AFAICT. Shouldn't be a huge deal to fix up. What's important is, this looks ready to merge so our brave master testers can try to break it in prod. 😁
|
Hmm, I'm tempted to dismiss my approving review because 1) this isn't squashed/rebased and 2) there actually are unresolved conversations about placeholders and docs. @Exirel take another look this weekend? I don't know when your work trip is done. Edit, from IRC: |
dbb5b52 to
de66564
Compare
This part: done. I didn't add anything yet, because I think it would be better for you if you wanted to check the diff between what you approved so far, and what will come next (docstrings, placeholders, etc.). |
I'm about to push a commit with just For other placeholders, I need to think a bit more about it: can we assume the time left will be in seconds, and the rate limit in seconds as well? Like, if the rate limit is something like 60s/3600s, should we be smart about it? Should we provide a human readable version of that? (Using cool functions from This leads me to think that, maybe, I could just commit what I have, and consider this PR "done": anyone could jump into the code and add the new placeholders as they wanted, with a separate/dedicated conversation about it, without all the other diff around. |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a small bit of collateral damage from the rebase happened; see line note.
We can certainly defer adding more kinds of placeholder for a later PR, but I really would like this patch to document the ones that it does add. 🙂
4d5b8c0 to
f4705e8
Compare
Co-authored-by: dgw <[email protected]>
f4705e8 to
605b586
Compare
|
Squashed & rebased, as per @dgw's validation on IRC. |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably test this one last time on a real bot before merging, but that doesn't mean I can't approve it first.

Description
Fix #830 by adding an optional message and three new decorators for specific rate limit messages (user, channel, and server).
Checklist
make qa(runsmake qualityandmake test)