-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
Fixed error when using async function based view in url path function #1887
Conversation
django-stubs/urls/conf.pyi
Outdated
@@ -10,6 +10,7 @@ from ..conf.urls import IncludedURLConf | |||
from ..http.response import HttpResponseBase | |||
|
|||
_URLConf: TypeAlias = str | ModuleType | Sequence[_AnyURL] | |||
_ResponseType: TypeAlias = HttpResponseBase | Coroutine[Any, Any, HttpResponseBase] | Coroutine[Any, Any, None] |
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.
Sorry, I don't think that adding Coroutine[Any, Any, None]
is correct.
In your case:
async def async_view():
return HttpResponse(status=200)
it does not return None
, it returns HttpResponse
.
The correct fix is:
async def async_view() -> HttpResponse:
return HttpResponse(status=200)
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.
Yes, I agree with you!)
I changed None to HttpResponse
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.
I still think that this change is not required, since Coroutine
is covariant on the return type: https://github.com/python/typeshed/blob/16933b838eef7be92ee02f66b87aa1a7532cee63/stdlib/typing.pyi#L442
We can add it, check how everything will work after if you don't need to delete it after |
Assigned to @sobolevn. If you don't want to lead the review here, feel free to unassign yourself. |
@ashm-tech do you want to continue the changes here? If yes, I'll make sure to help out with reviewing. I think what we need is a new Have a look at this #1874 (comment) to see what I mean. We should also add a test for the new overload, anywhere in https://github.com/typeddjango/django-stubs/blob/6f7a19cf26257fb2da1c65c12a72807f9154dfe7/tests/typecheck/urls/test_conf.yml. I believe that will help trying out the changes. |
Oh, sorry, just realised that this PR was superseded by #2085 |
Closes #1874