python
[django-filter] Absorb django-filter-stubs into typeshed
As described in #10918, I have added stubs from django-filter-stubs project. Upstream issue: https://github.com/DavisRayM/django-filter-stubs/issues/13
Please don't use squash merge to merge this (unless there's explicit decision to reconsider #10918). I have preserved original commit authors. There really weren't many changes in stubs files.
- Closes #10918
- Closes https://github.com/DavisRayM/django-filter-stubs/issues/13
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This should be more or less ready for initial merge. If we can get this merged, I can look into the stubtest allowlist issues separately. Unless you think I should fix them immediately here?
Some questions:
-
The stubs depend on
django-stubsanddjangorestframework-stubs. If we continue with this merge, these need to be added tostub_uploaderallowlist. Nikita Sobolev can vouch for the fact that these dependencies aren't evil :smiling_imp:Opened PR: https://github.com/typeshed-internal/stub_uploader/pull/111
-
stubtestgets exceptiondjango.core.exceptions.ImproperlyConfiguredfrom poking around in somedjango-filterinternals. I could just allowlist these.But if we could somehow set
DJANGO_SETTINGS_MODULEenv variable for the stubtest run (to some empty Python module even), then we wouldn't need to suppress them. Is there any precedent to this? -
Upstream used
Anyfor incomplete types. Should I convert all of these to_typeshed.Incompleteor not bother with this?
(Writing from my phone, I have limited access to a laptop with Internet until Tuesday)
In general, I think it's fine to tackle issues with these stubs in followup PRs, now you've got CI passing (thanks!). Most things in stubs/peewee/ are still unannotated; we still have many allowlist entries for types-redis — django-filter will hardly be unique in its stubs not being quite up to the standard of, say, our stdlib stubs.
For your question (1) — I can't remember off the top of my head what the exact rules are, but unfortunately adding django-stubs and djangorestframework-stubs to the stub_uploader allowlist might not be sufficient to get CI passing here. I think the stub_uploader might also check that all external dependencies listed in the requires field are actually dependencies of the runtime django-filter package, as an additional security check. I might be misremembering, though.
For your question (2), I think we're reasonably open to adding special-casing for specific stubs packages if it means we can run stubtest on them in CI. If you look at tests/stubtest_third_party.py, we already have some special-casing hardcoded into the script so that we can run stubtest on types-uwisgi in CI
For your question (1) [...] unfortunately adding
django-stubsanddjangorestframework-stubsto the stub_uploader allowlist might not be sufficient to get CI passinghere. I think the stub_uploader might also check that all external dependencies listed in the requires field are actually dependencies of the runtime django-filter package
Yep, that's also mentioned in https://github.com/typeshed-internal/stub_uploader/issues/90#issue-1587077892
Ideally these wouldn't be hard dependencies of the pacakge, but would only be installed in CI when testing. But there seems to be no such option currently?
Users of django who want to use types-django-filter are likely to have Django's stubs installed anyway. And if they don't, then types-django-filter is just less useful, but still better than nothing.
There are two different stubs packages for Django: django-stubs and django-types and we shouldn't force that choice.
I think anything else we can fix/improve afterwards, but the stub_uploader error is a blocker: https://github.com/python/typeshed/pull/10919#issuecomment-1774068623
One idea would be to create a new METADATA.toml option e.g. test_requires = [...] that's only used in CI and not subject to the strict checks as requires = [...] is. What do you think?
One idea would be to create a new
METADATA.tomloption e.g.test_requires = [...]that's only used in CI and not subject to the strict checks asrequires = [...]is. What do you think?
I'd prefer to avoid doing that, if at all possible. In 99% of cases in typeshed, if an external package is needed to make our tests pass, it's also needed for the stubs to make sense when they're installed by a user. I worry that by adding this option to our METADATA.toml files, we'd encourage people to add "test-only" dependencies which would mean our stubs would start behaving very differently under the conditions of our tests than they actually do when installed by users. We want to avoid that -- we want our tests to represent as closely as possible how our stubs packages behave when they're installed by users.
I believe that if a stubs package from typeshed imports a type that mypy can't resolve (because a dependency is missing), the imported type will just get silently resolved as Any -- mypy won't complain about it! So the user will get no warning that the stubs they installed aren't working as intended.
For the specific case of types-django-filter, maybe it's extremely unlikely that people will have types-django-filter installed without having django-stubs or django-types installed. But... that still feels like a risk I'm not willing to take -- people do crazy things on the internet :)
I'm going to take a look now and see if there are any other ways of solving this problem.
Many months have gone past. Has anything changed in terms of introducing a dependency on django-stubs? Or maybe some fresh ideas to solve this blocker?
There are a few other Django-related or djangorestframework-related projects I might want to provide stubs for, but no point in undertaking that work until this blocker has been cleared. And maintaining a 3rd party stubs project outside of typeshed is a bigger commitment than I want to undertake.
@intgr Sorry for the long process. I've added the Django stubs to the stub uploader allowlist. I see this as uncritical. There are some type errors now, though.
Thanks for helping push this forward.
Note that stub_uploader tests are giving the following error:
E stub_uploader.metadata.InvalidRequires: Expected dependency django-stubs to be listed in django-filter's requires_dist or the sdist's *.egg-info/requires.txt
This blocker was described in https://github.com/typeshed-internal/stub_uploader/pull/111#issuecomment-1774072444 / https://github.com/typeshed-internal/stub_uploader/issues/90#issue-1587077892
Do you have an idea what to do about this error? I suspect django-filter upstream will not want to add django-stubs to its dependencies.
@intgr maybe we can try try to make a PR with django-stubs as a dependency to django-filter?
Sorry for the inaction here. I feel we should be able to address this with changes in stub-uploader. (Maybe a rule that if you depend on X at runtime, the stubs may depends on X-stubs, for some trusted values of X?)
Sorry for the inaction here. I feel we should be able to address this with changes in stub-uploader. (Maybe a rule that if you depend on X at runtime, the stubs may depends on X-stubs, for some trusted values of X?)
This has been addressed in https://github.com/typeshed-internal/stub_uploader/pull/152. Perhaps someone can merge main to trigger ci to see if it works.
It's a mystery to me, why Pyright fails on this file (only with older Pythons)... I didn't even touch it :smile:
/home/runner/work/typeshed/typeshed/stubs/setuptools/setuptools/compat/py310.pyi:9:81 - error: Unnecessary "# pyright: ignore" rule: "reportMissingImports" (reportUnnecessaryTypeIgnoreComment)
It's a mystery to me, why Pyright fails on this file (only with older Pythons)... I didn't even touch it 😄
https://github.com/python/typeshed/pull/13101#issuecomment-2498572819
See also my suggested solution for the pyright issue: https://github.com/python/typeshed/pull/13101#issuecomment-2498615639
(But it should be done in a separate PR to this one)
See also my suggested solution for the pyright issue: #13101 (comment)
(But it should be done in a separate PR to this one)
#13280
Nice, this now passes CI.
As a reminder, Please don't use squash merge to merge this (unless there's explicit decision to reconsider #10918). I have preserved original commit authors in the commits here.