-
-
Notifications
You must be signed in to change notification settings - Fork 379
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
chore: Adds deprecation warnings for app
parameter when using to_asgi_response
method of responses
#2268
Conversation
…esponse method of responses
…esponse method of responses
Refactor/2217
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
There are some changes required, which I detailed in a comment. Feel free to ask should you have any questions regarding the implementation of this!
…dated the type for app to allow for it to be None, replaced usage of app in to_asgi_response to be None
fix: made the app deprecation calls only trigger when app is used
…ate_enginer, added it
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2268 |
@provinzkraut just pinging you for another review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this looks correct! A few minor things left to address (=
Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
please resolve the last comment @wevonosky |
@Goldziher and @provinzkraut I left a comment on why I had to modify that test. @all-contributors please add @wevonosky |
I couldn't determine any contributions to add, did you specify any contributions? |
@all-contributors please add @wevonosky for code |
I've put up a pull request to add @wevonosky! 🎉 |
Pull Request Checklist
Description
This PR adds deprecation warnings for the use of
app
in theto_asgi_response
methods of responses. Theapp
parameter is redundant since therequest
parameter has the associated app available.Close Issue(s)
Closes #2217