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

ratelimit.exceeded? checks >= rather than > #27

Open
oehlschl opened this issue Apr 12, 2017 · 1 comment
Open

ratelimit.exceeded? checks >= rather than > #27

oehlschl opened this issue Apr 12, 2017 · 1 comment

Comments

@oehlschl
Copy link

This is not a bug per se, but the behavior is unexpected; exceeded? returns true AT the threshold in addition to over it, which is unintuitive. This is reinforced by the language in the readme, which states that "the following code checks if the currently rate is over 10 executions in the last 30 seconds or not. ratelimit.exceeded?(phone_number, threshold: 10, interval: 30)"; in reality, the code checks if the rate is over 9 executions / equal-to-or-over 10 executions in the last 30 seconds.

The consequences of this depend on where .add() is called, but I personally feel like this line should be > rather than >=:

return count(subject, options[:interval]) >= options[:threshold]

RateLimit.js does not implement exceeded, but the example linked from your readme also suggests > over >=: https://gist.github.com/chriso/54dd46b03155fcf555adccea822193da#get-the-code

I can work around this in my implementation, but it thought this was worth mentioning. Otherwise, thanks for the great gem.

@oehlschl
Copy link
Author

I can also submit a PR if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant