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

new: ✨ add ASGI middleware alternative #113

Merged
merged 12 commits into from
Nov 7, 2022

Conversation

thentgesMindee
Copy link
Collaborator

fix #98

Might be easier for people reviewing to explain the different commits:

  • first commit adds the new ASGI middleware
  • second one update tests so that it test both the HTTP and ASGI middlewares (using pytest parametrize and fixtures)
  • third one is a refactoring commit, to avoid duplicating too much code between the HTTP and the ASGI middleware

Might be easier to first review the first two commits, then the refactoring one (to avoid seeing too many changes at once)

Let me know what do you think about this 😄

Note: maybe I could do a bit of refactoring between _inject_headers and _inject_asgi_headers as well ?

@laurentS
Copy link
Owner

Thanks for this PR @thentgesMindee ! Just to say I have no time to look into it this week (nor next, most likely), but there are other contributors to this repo who might get to it before then.

@thentgesMindee
Copy link
Collaborator Author

@laurentS alright thanks a lot for the information, no worries 😄
I see you enabled the github actions check on the branch and they fail because some changes were made on master and not rebased on my branch, gonna fix this right away

@thentgesMindee thentgesMindee force-pushed the asgi-middleware branch 2 times, most recently from 2d14dda to 6ef3e55 Compare September 30, 2022 09:36
@thentgesMindee
Copy link
Collaborator Author

rebase made, + commit to update newly added tests
all checks should pass this time 😄

Copy link
Owner

@laurentS laurentS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing this. I suggested a few type annotations, hoping to at least get mypy to look into each function.
I think it'd be nice to give a minimal example of each usage in the docs as well, and maybe add a line explaining how you'd choose one or the other middleware.

slowapi/extension.py Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
slowapi/middleware.py Outdated Show resolved Hide resolved
@thentgesMindee
Copy link
Collaborator Author

Hi thanks for the review @laurentS !
I spotted a bug in my middleware, I'll fix it then handle your review requests

@thentgesMindee
Copy link
Collaborator Author

Just as an update, committed the fix I was talking about, as well as type annotations change requests.
will do some more commits either today or tomorrow for docstring and documentation 👌

@thentgesMindee
Copy link
Collaborator Author

Sorry for the delay, but I finally managed to find some time to:

  • add support for both asynchronous or synchronous error handlers in the ASGI Middleware
  • add a small documentation about choosing between ASGI and WSGI middleware
  • some docstrings

Let me know what do you think about it

@thentgesMindee
Copy link
Collaborator Author

Hi @laurentS , sorry for being impatient, any chance you could review the requested changes ?
Don't want to rush you, just want to make sure this is not lost in time

@laurentS
Copy link
Owner

laurentS commented Nov 3, 2022

@thentgesMindee sorry for the lag. Things are a bit busy everywhere. I just approved the CI run, hadn't noticed this was waiting for a button click, and it failed on typing. Can you check and correct?

@thentgesMindee
Copy link
Collaborator Author

Sure no worries take your time, don't want to rush you, as I said I just wanted to make sure the PR wasn't forgotten.
Will have a look to the failure and ping you back when I resolve it

@thentgesMindee
Copy link
Collaborator Author

@laurentS type annotations were updated (just left a # type: ignore, but the other ones are fixed)

@thentgesMindee
Copy link
Collaborator Author

Just saw that the CI is still failing because of some unused imports, gonna fix it tomorrow!

laurentS
laurentS previously approved these changes Nov 7, 2022
Copy link
Owner

@laurentS laurentS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Just a suggestion in _get_route_name to use a more modern style, otherwise all LGTM!

slowapi/middleware.py Outdated Show resolved Hide resolved
@thentgesMindee
Copy link
Collaborator Author

Thanks a lot for the review! rebased my branch to be up-to-date with current master, and added the last change about _get_route_name. All tests are passing.
Could you re-approve (can't approve my own PR, and seems that my force-push reset the old approvals) so that we merge this ?
Thanks again 🙏

1 similar comment
@thentgesMindee
Copy link
Collaborator Author

Thanks a lot for the review! rebased my branch to be up-to-date with current master, and added the last change about _get_route_name. All tests are passing.
Could you re-approve (can't approve my own PR, and seems that my force-push reset the old approvals) so that we merge this ?
Thanks again 🙏

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.

Move to ASGI middleware implementation?
2 participants