-
-
Notifications
You must be signed in to change notification settings - Fork 485
add mypy plugin support for Django's storage framework #2680
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,21 @@ | ||
from typing import Any | ||
from typing import Any, TypedDict, type_check_only | ||
|
||
from django.core.exceptions import ImproperlyConfigured | ||
from django.utils.functional import cached_property | ||
from typing_extensions import NotRequired | ||
|
||
from .base import Storage | ||
|
||
@type_check_only | ||
class _StorageConfig(TypedDict): | ||
BACKEND: str | ||
OPTIONS: NotRequired[dict[str, Any]] | ||
|
||
class InvalidStorageError(ImproperlyConfigured): ... | ||
|
||
class StorageHandler: | ||
def __init__(self, backends: dict[str, Storage] | None = None) -> None: ... | ||
def __init__(self, backends: dict[str, _StorageConfig] | None = None) -> None: ... | ||
@cached_property | ||
def backends(self) -> dict[str, Storage]: ... | ||
def backends(self) -> dict[str, _StorageConfig]: ... | ||
def __getitem__(self, alias: str) -> Storage: ... | ||
def create_storage(self, params: dict[str, Any]) -> Storage: ... | ||
def create_storage(self, params: _StorageConfig) -> Storage: ... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
from mypy.checker import TypeChecker | ||
from mypy.plugin import AnalyzeTypeContext, MethodContext | ||
from mypy.semanal import SemanticAnalyzer | ||
from mypy.typeanal import TypeAnalyser | ||
from mypy.types import Instance, PlaceholderType, UninhabitedType, get_proper_type | ||
from mypy.types import Type as MypyType | ||
from mypy.typevars import fill_typevars | ||
|
||
from mypy_django_plugin.django.context import DjangoContext | ||
from mypy_django_plugin.lib import helpers | ||
|
||
|
||
def get_storage_backend(alias: str, django_context: DjangoContext) -> str | None: | ||
"Defensively look for a settings.STORAGES by its alias." | ||
|
||
try: | ||
fullname = django_context.settings.STORAGES[alias]["BACKEND"] | ||
if not isinstance(fullname, str) or "." not in fullname: | ||
return None | ||
|
||
return fullname | ||
except (KeyError, TypeError): | ||
return None | ||
|
||
|
||
def get_storage(ctx: AnalyzeTypeContext, alias: str, django_context: DjangoContext) -> MypyType: | ||
""" | ||
Get a storage type by its alias, but do not fail if it cannot be found since this is resolving an internal type-var, | ||
and errors would be reported in the type stubs. | ||
""" | ||
|
||
assert isinstance(ctx.api, TypeAnalyser) | ||
assert isinstance(ctx.api.api, SemanticAnalyzer) | ||
|
||
if fullname := get_storage_backend(alias, django_context): | ||
if type_info := helpers.lookup_fully_qualified_typeinfo(ctx.api.api, fullname): | ||
return fill_typevars(type_info) | ||
|
||
if not ctx.api.api.final_iteration: | ||
ctx.api.api.defer() | ||
return PlaceholderType(fullname=fullname, args=[], line=ctx.context.line) | ||
|
||
return ctx.type | ||
|
||
|
||
def extract_proper_type_for_getitem(ctx: MethodContext, django_context: DjangoContext) -> MypyType: | ||
""" | ||
Provide type information for `StorageHandler.__getitem__` when providing a literal value. | ||
""" | ||
|
||
assert isinstance(ctx.api, TypeChecker) | ||
|
||
if ctx.arg_types: | ||
alias_type = get_proper_type(ctx.arg_types[0][0]) | ||
|
||
if ( | ||
isinstance(alias_type, Instance) | ||
and (alias_literal := alias_type.last_known_value) | ||
and isinstance(alias := alias_literal.value, str) | ||
): | ||
if alias not in django_context.settings.STORAGES: | ||
ctx.api.fail(f'Could not find config for "{alias}" in settings.STORAGES.', ctx.context) | ||
|
||
elif fullname := get_storage_backend(alias, django_context): | ||
type_info = helpers.lookup_fully_qualified_typeinfo(ctx.api, fullname) | ||
assert type_info | ||
return fill_typevars(type_info) | ||
|
||
else: | ||
ctx.api.fail(f'"{alias}" in settings.STORAGES is improperly configured.', ctx.context) | ||
|
||
return UninhabitedType() | ||
|
||
return ctx.default_return_type |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from django.contrib.staticfiles.storage import ConfiguredStorage, StaticFilesStorage, staticfiles_storage | ||
from typing_extensions import assert_type | ||
|
||
# The plugin can figure out what these are (but pyright can't): | ||
assert_type(staticfiles_storage, StaticFilesStorage) # pyright: ignore[reportAssertTypeFailure] | ||
|
||
# what pyright thinks these are: | ||
assert_type(staticfiles_storage, ConfiguredStorage) # mypy: ignore[assert-type] | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
from django.core.files.storage import DefaultStorage, FileSystemStorage, Storage, default_storage, storages | ||
from typing_extensions import assert_type | ||
|
||
# The plugin can figure out what these are (but pyright can't): | ||
assert_type(default_storage, FileSystemStorage) # pyright: ignore[reportAssertTypeFailure] | ||
assert_type(storages["default"], FileSystemStorage) # pyright: ignore[reportAssertTypeFailure] | ||
|
||
# what pyright thinks these are: | ||
assert_type(default_storage, DefaultStorage) # mypy: ignore[assert-type] | ||
assert_type(storages["default"], Storage) # mypy: ignore[assert-type] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
- case: test_staticfiles_storage_defaults | ||
main: | | ||
from django.contrib.staticfiles.storage import staticfiles_storage | ||
|
||
reveal_type(staticfiles_storage) # N: Revealed type is "django.contrib.staticfiles.storage.StaticFilesStorage" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
- case: test_storage_defaults | ||
main: | | ||
from django.core.files.storage import default_storage, storages | ||
|
||
reveal_type(default_storage) # N: Revealed type is "django.core.files.storage.filesystem.FileSystemStorage" | ||
reveal_type(storages["default"]) # N: Revealed type is "django.core.files.storage.filesystem.FileSystemStorage" | ||
reveal_type(storages["staticfiles"]) # N: Revealed type is "django.contrib.staticfiles.storage.StaticFilesStorage" | ||
|
||
- case: test_custom_storages | ||
main: | | ||
from django.core.files.storage import default_storage, storages | ||
|
||
reveal_type(default_storage) # N: Revealed type is "myapp.storage.MyDefaultStorage" | ||
reveal_type(storages["default"]) # N: Revealed type is "myapp.storage.MyDefaultStorage" | ||
reveal_type(storages["custom"]) # N: Revealed type is "myapp.storage.MyStorage" | ||
reveal_type(storages["staticfiles"]) # N: Revealed type is "django.contrib.staticfiles.storage.StaticFilesStorage" | ||
|
||
custom_settings: | | ||
from django.conf.global_settings import STORAGES as DEFAULT_STORAGES | ||
|
||
STORAGES = { | ||
**DEFAULT_STORAGES, | ||
"default": {"BACKEND": "myapp.storage.MyDefaultStorage"}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will happen if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will probably break, and I figured Django checked this, but I have made the plugin more defensive. |
||
"custom": { | ||
"BACKEND": "myapp.storage.MyStorage", | ||
"OPTIONS": {"option_enabled": False, "key": "test"}, | ||
} | ||
} | ||
|
||
files: | ||
- path: myapp/storage.py | ||
content: | | ||
from django.core.files.storage import Storage | ||
|
||
class MyDefaultStorage(Storage): | ||
pass | ||
|
||
class MyStorage(Storage): | ||
pass | ||
|
||
- case: test_improperly_configured_storages | ||
main: | | ||
from django.core.files.storage import default_storage, storages | ||
|
||
reveal_type(default_storage) # N: Revealed type is "_DefaultStorage?" | ||
reveal_type(storages["default"]) # E: "default" in settings.STORAGES is improperly configured. [misc] # N: Revealed type is "Never" | ||
reveal_type(storages["custom"]) # E: "custom" in settings.STORAGES is improperly configured. [misc] # N: Revealed type is "Never" | ||
reveal_type(storages["custom_two"]) # E: "custom_two" in settings.STORAGES is improperly configured. [misc] # N: Revealed type is "Never" | ||
reveal_type(storages["staticfiles"]) # E: Could not find config for "staticfiles" in settings.STORAGES. [misc] # N: Revealed type is "Never" | ||
|
||
custom_settings: | | ||
STORAGES = { | ||
"custom": {"BACKEND": "MyStorage"}, | ||
"custom_two": ["MyStorage"], | ||
"default": True, | ||
# "staticfiles" is missing | ||
} |
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.
so, basically, right now
pyright
will inferConfiguredStorage
instead ofAny
and existing code can break, due to the fact thatConfiguredStorage
is aLazyObject
which does not really have right attributes.How can we fix that?
Uh oh!
There was an error while loading. Please reload this page.
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.
It wasn't
Any
before, but it can be if that's better... I just looked and mypy can't have its own type ignores, so I'm not sure how to ever make this work since mypy will always know more about the object than pyright does (even if it reportsAny
for pyright the assertion will fail, right?).