slowapi
slowapi copied to clipboard
laurentS
feat: allow Requests to be sent to exempt_when
Creates a way to use the request object in the exempt_when function.
Now allows functions like this to be used:
def test_exempt(x) -> bool:
print(x.headers.get("User-Agent"))
return False
Fixes #155
Lines 702 and 706 were automatically removed by my linter because they are unused. Let me know if you want them back. Also, let me know if you want ruff added to CI for code linting.
+1
Today we return None as part of the key_func function in order to bypass the limit, instead of using the exempt_when function. Then we get the error kipping limit: %s. Empty value found in parameters..
This PR would enable us to use exempt_when and make the code cleaner.
@laurentS anything else you want to see here for this to be mergeable?
@colin99d thanks for submitting this PR! I just took a quick look, but I saw the PR changes the arguments to a method that's probably public, which means it would probably break some users' code (at least their typechecks would complain). Can the extra arg be made optional to avoid that?
@laurentS anything else you want to see before this is merged?
@colin99d I merged @Rested 's suggestions, can you just check that tests pass, etc..., and I think we're good to go then.
@laurentS it looks like the change by @Rested causes all the tests to fail. Any idea how to fix it?
Also, I got a little carried away and switch from Flake8 to Ruff. Its pretty much Flake8 but way better in every way.
Maybe we should try to do one thing per PR. Let's focus this one on the feature you initially proposed, and we can add ruff in a separate one.
I won't have time to look into this for the next few days, but it looks like there's a bug in the github ci file preventing the checks from running, no?
I reverted ruff changes. All tests fail because of this line:
self._exempt_when_takes_request = len(inspect.signature(self.exempt_when).parameters) == 1
I reverted ruff changes. All tests fail because of this line:
self._exempt_when_takes_request = len(inspect.signature(self.exempt_when).parameters) == 1
It just needs reformatting. Can you run black on your code?
I can't commit to your branch, so you'll have to do it.
It should probably look like this:
self._exempt_when_takes_request = (
len(inspect.signature(self.exempt_when).parameters) == 1
)
Not related to this PR but maybe we could implement pre-commit on the repo, so that anyone wanting to contribute would just need to do one pre-commit install when first cloning their repo to have all code formatting applied automatically when they commit
@laurentS are we waiting on anything else here?
I think CI checks should be green before merging :) Logs seem to have been removed by github, but I think there was something pretty clear in there as to what needs fixing.
Can you please rerun them so I can see what it is?
I can't seem to rerun CI on github, weird. I pulled your changes locally, try poetry run mypy . and you'll see it is not happy about an unhandled case (when exempt_when is None) in wrappers.py:33.
And in general, you should be able to trigger a CI run by pushing an empty commit (or just doing some cosmetic change in the code).
@colin99d I would assume it should be added to the 'CHANGELOG.md'.
But mostly when doing the next release, so feels like this could merged @laurentS / @Rested ?
@laurentS yeah just let me know exactly where you want this note added so we can get this merged.
Yes, the changelog is a good place for that. If you make sure to include "breaking change" in the wording, it'll probably help users as well. Thanks for your contribution!
Looks great! think adding some usage docs here https://github.com/laurentS/slowapi/blob/6547679463b6a6fa87d9e254a8558bbc6075e263/slowapi/extension.py#L807 and https://github.com/laurentS/slowapi/blob/master/docs/examples.md and a small test around here https://github.com/laurentS/slowapi/blob/6547679463b6a6fa87d9e254a8558bbc6075e263/tests/test_starlette_extension.py#L195 would be ideal before merging!
@laurentS @Rested improved the docstring and added a comprehensive set of tests, can we merge this now?
@laurentS any plans to publish a new version to Pypi with this change (and a few others that have landed since the v0.1.9 release from February)?