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

[8.x] Change argument order of perMinues() rate limit method #36568

Closed
wants to merge 1 commit into from
Closed

[8.x] Change argument order of perMinues() rate limit method #36568

wants to merge 1 commit into from

Conversation

intrepidws
Copy link
Contributor

I created the pull request (#36352) that introduced the perMinutes() rate limit method, to allow greater specificity than the existing rate limit methods allowed. Somehow, during the merge process, it seems like a commit (86d0a5c) was introduced that swapped the order of the arguments.

As a result, the behavior of perMinutes() differs from that of the previously existing methods, perMinute(), perDay() and perHour(). Perhaps I'm missing something obvious here but it seems like the argument order for perMinutes() should match that of the other rate limit methods. This PR changes the order of the arguments so that they match.

As a matter of example, my application had the following code to rate limiting a job to run a maximum of once every 30 minutes:

return Limit::perHour(1, 0.50);

After the perMinutes() method was introduced, I expected to be able to change my code as such:

return Limit::perMinutes(1, 30);

@driesvints driesvints changed the title Change argument order of perMinues() rate limit method [8.x] Change argument order of perMinues() rate limit method Mar 12, 2021
@GrahamCampbell
Copy link
Member

This is a major breaking change.

@intrepidws
Copy link
Contributor Author

Yea, you're right. Can we PR this to 9.x instead? And if it's gonna be a breaking change anyway, it would probably make sense to get rid of this new perMinutes() method entirely and simply allow the perMinute() method to accept a minutes argument (making it similar in both naming convention and style) to perHour() and perDay().

@GrahamCampbell
Copy link
Member

We also have to decide if the change is worth it. Upgrading is hard because people probably won't notice this change, since nothing obviously breaks.

@intrepidws
Copy link
Contributor Author

Understood. My fear is that people will, naturally I think, assume that perMinutes() has the same argument order as perHour() and perDay(). When I changed my code from perHour() to perMinutes(), I was bit by the swapped argument order and instead of having my job run 1 time every 30 minutes, it was able to run 30 times every 1 minute. A huge difference, obviously.

I'm still confused as to how the arguments got swapped in the first place honestly, but the end result is an inconsistent developer experience with real consequences.

I suppose another option is to keep perMinutes() as is, and introduce the breaking change in 9.x to make perMinute() consistent with perHour() and perDay().

Anyway, I'm happy to alter this PR however people agree is best - if at all.

@taylorotwell
Copy link
Member

I think the current order is fine. I believe I may have even changed it. To me it reads naturally say perMinutes(NUMBER_OF_MINUTES... ... since I just said "per minutes" I expect to specify how many minutes first, and then how many requests can take place within that number of minutes.

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

Successfully merging this pull request may close these issues.

3 participants