add mypy plugin support for Django's storage framework #2680
Conversation
terencehonles
force-pushed
the
add-storage-support-to-mypy-plugin
branch
4 times, most recently
from
792c3be to
8e45e7e
Compare
mypy_django_plugin/main.py
Outdated
| # for settings.STORAGES["staticfiles"] | ||
| if file.fullname == "django.contrib.staticfiles.storage": | ||
| return [ | ||
| self._new_dependency(self.django_context.settings.STORAGES["staticfiles"]["BACKEND"].rsplit(".", 1)[0]) |
There was a problem hiding this comment.
They are both set by default, and the staticfiles app has a check that would raise ImproperlyConfigured, but it doesn't look like default has one. For Django 5.0 (in the depreciation period for DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings), it puts those in the appropriate STORAGES entries, so those will be there and correct.
Having that said, I'll just make these defensive, and if type-checking an invalid project it will just no-op (since in this location it can't error).
|
|
||
| STORAGES = { | ||
| **DEFAULT_STORAGES, | ||
| "default": {"BACKEND": "myapp.storage.MyDefaultStorage"}, |
There was a problem hiding this comment.
What will happen if MyDefaultStorage is something invalid? Like instance of value 1 or 'abc'
There was a problem hiding this comment.
It will probably break, and I figured Django checked this, but I have made the plugin more defensive.
terencehonles
force-pushed
the
add-storage-support-to-mypy-plugin
branch
2 times, most recently
from
1923a7d to
8ccd6c4
Compare
|
|
||
| try: | ||
| fullname = django_context.settings.STORAGES[alias]["BACKEND"] | ||
| assert isinstance(fullname, str) and "." in fullname |
There was a problem hiding this comment.
Please, convert an assert to if, since we catch AssertionError
There was a problem hiding this comment.
I am also catching assertion error. I figured this was better than if check: raise ValueError, but do you not agree?
There was a problem hiding this comment.
if not isinstance(fullname, str) or "." not in fullname:
return None?
There was a problem hiding this comment.
I forgot that this code was no longer doing anything in the exception handler (since that was omitted in your quote). It was previously reporting a failure and that's why this code looked like this. I can adjust.
There was a problem hiding this comment.
I mostly ask, because assert can be removed from code with -OO, it is fine for sanity-checks and type-narrowing, but I don't think that we can base logics on it :)
terencehonles
force-pushed
the
add-storage-support-to-mypy-plugin
branch
from
8ccd6c4 to
8525c98
Compare
I'm not really sure how those are supposed to work or what I am supposed to put there, but hopefully that's sufficient 🤷 |
terencehonles
force-pushed
the
add-storage-support-to-mypy-plugin
branch
2 times, most recently
from
6bc6b86 to
3f760ed
Compare
This change correctly resolves ``django.core.files.storage.default_storage`` and ``django.contrib.staticfiles.storage.staticfiles_storage`` which are configured in the project's Django settings. When ``django.core.files.storage.storages``, which has a dict like interface, is accessed with a literal value, then the type of the appropriate storage is returned.
terencehonles
force-pushed
the
add-storage-support-to-mypy-plugin
branch
from
3f760ed to
dd8f041
Compare
| assert_type(staticfiles_storage, StaticFilesStorage) # pyright: ignore[reportAssertTypeFailure] | ||
|
|
||
| # what pyright thinks these are: | ||
| assert_type(staticfiles_storage, ConfiguredStorage) # mypy: ignore[assert-type] |
There was a problem hiding this comment.
so, basically, right now pyright will infer ConfiguredStorage instead of Any and existing code can break, due to the fact that ConfiguredStorage is a LazyObject which does not really have right attributes.
How can we fix that?
There was a problem hiding this comment.
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 reports Any for pyright the assertion will fail, right?).
I have made things!
This change correctly resolves
django.core.files.storage.default_storageanddjango.contrib.staticfiles.storage.staticfiles_storagewhich are configured in the project's Django settings.When
django.core.files.storage.storages, which has a dict like interface, is accessed with a literal value, then the type of the appropriate storage is returned.Related issues