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

chore: Adds deprecation warnings for app parameter when using to_asgi_response method of responses #2268

Merged
merged 11 commits into from
Sep 9, 2023

Conversation

wevonosky
Copy link
Contributor

Pull Request Checklist

  • New code has 100% test coverage
  • (If applicable) The prose documentation has been updated to reflect the changes introduced by this PR
  • (If applicable) The reference documentation has been updated to reflect the changes introduced by this PR

Description

This PR adds deprecation warnings for the use of app in the to_asgi_response methods of responses. The app parameter is redundant since the request parameter has the associated app available.

Close Issue(s)

Closes #2217

@wevonosky wevonosky requested review from a team as code owners September 1, 2023 13:48
Copy link
Member

@provinzkraut provinzkraut left a 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!

litestar/response/base.py Outdated Show resolved Hide resolved
wevonosky and others added 5 commits September 1, 2023 10:12
…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
@github-actions
Copy link

github-actions bot commented Sep 2, 2023

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/2268

@wevonosky
Copy link
Contributor Author

@provinzkraut just pinging you for another review :)

Copy link
Member

@provinzkraut provinzkraut left a 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 (=

litestar/response/base.py Outdated Show resolved Hide resolved
litestar/response/file.py Outdated Show resolved Hide resolved
litestar/response/redirect.py Outdated Show resolved Hide resolved
litestar/response/streaming.py Outdated Show resolved Hide resolved
litestar/response/template.py Outdated Show resolved Hide resolved
litestar/response/template.py Show resolved Hide resolved
Goldziher and others added 2 commits September 7, 2023 08:18
Co-authored-by: Janek Nouvertné <provinzkraut@posteo.de>
@Goldziher
Copy link
Contributor

please resolve the last comment @wevonosky

@wevonosky
Copy link
Contributor Author

@Goldziher and @provinzkraut I left a comment on why I had to modify that test.

@all-contributors please add @wevonosky

@allcontributors
Copy link
Contributor

@wevonosky

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@wevonosky
Copy link
Contributor Author

@all-contributors please add @wevonosky for code

@allcontributors
Copy link
Contributor

@wevonosky

I've put up a pull request to add @wevonosky! 🎉

@Goldziher Goldziher merged commit 488df7d into litestar-org:main Sep 9, 2023
16 checks passed
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.

Refactor: remove app parameter from Response.to_asgi_response
3 participants