Skip to content

django.shortcuts.redirect should use a type Literal overload for return value #378

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

Closed
silviogutierrez opened this issue Jun 1, 2020 · 2 comments · Fixed by #387
Closed
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@silviogutierrez
Copy link

silviogutierrez commented Jun 1, 2020

Bug report

What's wrong

As of a recent django-stubs release, and HttpResponseRedirect and HttpResponsePermanentRedirect are no longer compatible with each other (rightly so, they only share an ancestor but the latter does not inherit from the former).

This means to annotate a view's response, you need to use something like Union[TemplateResponse, HttpResponseRedirect, HttpResponsePermanentRedirect] for your typical https://en.wikipedia.org/wiki/Post/Redirect/Get . Obviously you could just annotate with HttpResponse but I would argue that's too weak.

How is that should be

Calling redirect with permanent=True should return one type, and permanent=False return another. This is possible with Literal types now, see https://mypy.readthedocs.io/en/stable/literal_types.html.

Happy to take a stab at this assuming we're ok with using newer MyPy features like that above. I'm not quite sure if we should import Literal from typing or from somewhere else if we support lower Python versions. Any guidance on these imports would be great.

Thanks for all the work on this library. It's really improved my Mypy experience.

System information

  • OS:
  • python version: 3.8
  • django version: 3.0
  • mypy version: 0.770
  • django-stubs version: 1.5.0
@silviogutierrez silviogutierrez added the bug Something isn't working label Jun 1, 2020
@sobolevn sobolevn added the help wanted Extra attention is needed label Jun 1, 2020
@kszmigiel
Copy link
Member

Hello @silviogutierrez and thanks for report!

Are you able to precisely point out django-stubs version in which this bug hadn't exist? It would help me a lot solving this.

@silviogutierrez
Copy link
Author

silviogutierrez commented Jun 1, 2020

@kszmigiel : sure thing.

1.2 had redirect returning HttpResponseRedirect only. In reality, this was incorrect as we know it can return HttpResponsePermanentRedirect too. See here:

https://github.com/typeddjango/django-stubs/blob/v1.2.0/django-stubs/shortcuts.pyi

1.3 was updated to return a Union of both types. Link here:

https://github.com/typeddjango/django-stubs/blob/v1.3.0/django-stubs/shortcuts.pyi

To be clear, it's not a bug per se. I think the issue just defaulted to bug. This is really more of an improvement to redirect. Apologies if I messed it up on my end.

@kszmigiel kszmigiel added enhancement New feature or request and removed bug Something isn't working labels Jun 3, 2020
@kszmigiel kszmigiel mentioned this issue Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Development

Successfully merging a pull request may close this issue.

3 participants