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

Fix ASGI compatibliity on Python 3.12 #911

Merged
merged 10 commits into from
Nov 14, 2023

Conversation

Royalflamejlh
Copy link
Contributor

From issue #908, changed middleware.py to reflect the changes made in adamchainz/django-htmx#381 as they had similar issues.

@adamchainz
Copy link
Owner

Did this fix the issue for you? Please also add a changelog note and include the asgiref >= 3.6.0 requirement in pyproject.toml.

@Royalflamejlh
Copy link
Contributor Author

Yes, this did fix the issue. I added a changelog note, but I'm not sure how to update the tests to get 100% coverage, also should I be adding the asgiref >= 3.6.0 under dependencies?

@captain828
Copy link

For the test coverage issue, have a look at the HTML coverage report in the artifacts section: https://github.com/adamchainz/django-cors-headers/actions/runs/6805112094?pr=911

image

So basically there is a test missing for the else branch there.

Comment on lines 65 to 67
result = self.get_response(request)
assert not isinstance(result, HttpResponseBase)
response = await result
Copy link
Owner

Choose a reason for hiding this comment

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

This section did not need changing. The change lead to insufficient test coverage.

It doesn't need changing because when __acall__ is called, we know we're in async mode, and that get_response will return a coroutine. Mypy just doesn't know that, hence the type-narrowing assert

@adamchainz
Copy link
Owner

Alright, I recreated the bug with an example project and have verified this fix works. Thank you both.

@adamchainz adamchainz changed the title Fix for ASGI Fix ASGI compatibliity on Python 3.12 Nov 14, 2023
@adamchainz adamchainz enabled auto-merge (squash) November 14, 2023 17:08
@adamchainz adamchainz merged commit 7fe1d62 into adamchainz:main Nov 14, 2023
7 checks passed
@adamchainz
Copy link
Owner

Released in 4.3.1.

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