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

parameters supporting lazy strings are not typed as accepting them #982

Closed
nils-van-zuijlen opened this issue May 2, 2023 · 6 comments
Closed
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@nils-van-zuijlen
Copy link
Contributor

Describe the bug
Updating to django-stubs to 1.13.0 and later, the lazy string functions from the translations module are declared as returning Promise objects instead of str.

Most classes / functions in drf_spectacular are typed as accepting str only, but work well with lazy strings, and are now flagged by mypy.

To Reproduce
Declare a response with the description set to a lazy translatable string.

from django.utils.translations import gettext_lazy as _
from drf_spectacular.utils import OpenApiResponse
OpenApiResponse(description=_("Parsing and validation errors."))

Typecheck the code with mypy and django-stubs 1.13.0 or later.

foo.py:3: error: Argument "description" to "OpenApiResponse" has incompatible type "_StrPromise"; expected "str"  [arg-type]

Expected behavior
No typecheck error is raised, the translatable arguments are declared as allowing StrOrPromise

References
https://github.com/typeddjango/django-stubs#why-am-i-getting-incompatible-argument-type-mentioning-_strpromise

@nils-van-zuijlen
Copy link
Contributor Author

This does not currently cause errors when type checking drf-spectacular itself because the django-stubs dependency was pinned to the version before that change: #693 (comment) and 799f394

But since that was more than one year ago, would it be possible to unpin it?

@tfranzel tfranzel added the enhancement New feature or request label May 7, 2023
@tfranzel
Copy link
Owner

tfranzel commented May 7, 2023

Yes, we should check and lift the stubs pinning.

Yes, accounting for promise str we should definitely support in the typing. Question is which type is the best here? We can't use something from the stubs package directly, as it may not be available in production. Conditional import might be an option.

Or maybe just _LazyStrType = Union[str, Promise[str]] and use that in utils.py. Looks like they basically do the same.

@nils-van-zuijlen
Copy link
Contributor Author

The django-stubs-ext could be added as a typecheck dependency and the import conditioned on typing.TYPE_CHECKING?

@tfranzel
Copy link
Owner

tfranzel commented May 7, 2023

Although I lean towards DIY for this seemingly simple thing, there is likely something more going on through the mypy django plugin. _StrPromise does not exist outside of mypy.

https://github.com/typeddjango/django-stubs/blob/master/django_stubs_ext/django_stubs_ext/aliases.py

We also have to make sure we don't break users relying on other static type checkers like pyright.

@nils-van-zuijlen
Copy link
Contributor Author

As they are defined in https://github.com/typeddjango/django-stubs/blob/master/django-stubs/utils/functional.pyi#L30 I don't see why they would not exist for those users.

And if I understand correctly typeddjango/django-stubs#579, pyright does not break when django-stubs are applied without extension.

Another option would be to try importing from django-stubs directly at typechecking time, and if it not available to define StrOrPromise as being str.

@tfranzel
Copy link
Owner

I know this is not exactly what was discussed, but I thought I give the minimal version a try before introducing conditional importing and type checking branching.

@nils-van-zuijlen, can you confirm this behaves as expected?

tfranzel added a commit that referenced this issue Jul 9, 2023
allow already supported lazy string in types #982
@tfranzel tfranzel added the fix confirmation pending issue has been fixed and confirmation from issue reporter is pending label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

2 participants