-
-
Notifications
You must be signed in to change notification settings - Fork 405
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, loader, plugin, rules: add NOTICE message to rate limits #2290
Conversation
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.
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. |
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. |
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 <dgw@technobabbl.es>
f4705e8
to
605b586
Compare
Squashed & rebased, as per @dgw's validation on IRC. |
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 quality
andmake test
)