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, loader, plugin, rules: add NOTICE message to rate limits #2290

Merged
merged 1 commit into from
Jul 16, 2022

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented May 29, 2022

Description

Fix #830 by adding an optional message and three new decorators for specific rate limit messages (user, channel, and server).

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 quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added the Feature label May 29, 2022
@Exirel Exirel added this to the 8.0.0 milestone May 29, 2022
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.

I haven't dived into the tests yet (there are a lot of them), but there might be enough to discuss already without them.

sopel/bot.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.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.

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.

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/loader.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugin.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
sopel/plugins/rules.py Outdated Show resolved Hide resolved
test/plugins/test_plugins_rules.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jun 12, 2022

I've modified func.rate into func.user_rate (and many little changes like that).

Also now there are conflicts... I didn't look at them yet.

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.

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

@dgw
Copy link
Member

dgw commented Jun 21, 2022

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: 16:15:43 <+Exirel> dgw: I'm too busy up until next week at least.

@Exirel Exirel force-pushed the rate-limit-notice branch from dbb5b52 to de66564 Compare June 25, 2022 07:44
@Exirel
Copy link
Contributor Author

Exirel commented Jun 25, 2022

  1. this isn't squashed/rebased

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

@Exirel
Copy link
Contributor Author

Exirel commented Jun 25, 2022

Since there's already some support for placeholders, what do you think of adding one for the time remaining until the rate limit expires? It's the main reason that I wished for this feature to be built in, all those years ago.

Regardless of the above, these placeholders don't appear to be documented anywhere.

I'm about to push a commit with just nick for user and global rate limits, nick and channel for channel rate limit.

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 tools.time for example?)

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.

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.

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

sopel/plugin.py Outdated Show resolved Hide resolved
test/test_bot.py Show resolved Hide resolved
@Exirel
Copy link
Contributor Author

Exirel commented Jul 10, 2022

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

I added the proper docstrings, and this little "see also" section in the doc:

image

@Exirel Exirel force-pushed the rate-limit-notice branch from 4d5b8c0 to f4705e8 Compare July 13, 2022 21:53
@Exirel Exirel force-pushed the rate-limit-notice branch from f4705e8 to 605b586 Compare July 13, 2022 21:56
@Exirel
Copy link
Contributor Author

Exirel commented Jul 13, 2022

Squashed & rebased, as per @dgw's validation on IRC.

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.

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.

@dgw dgw merged commit 03d2721 into sopel-irc:master Jul 16, 2022
@Exirel Exirel deleted the rate-limit-notice branch April 8, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin API: Add optional message to module.rate()
2 participants