[django-filter] Absorb django-filter-stubs into typeshed #10919

intgr · 2023-10-20T15:25:20Z

As described in #10918, I have added stubs from django-filter-stubs project. Upstream issue: DavisRayM/django-filter-stubs#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.

AlexWaygood · 2023-10-20T15:30:02Z

pre-commit.ci autofix

intgr · 2023-10-22T10:34:54Z

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-stubs and djangorestframework-stubs. If we continue with this merge, these need to be added to stub_uploader allowlist. Nikita Sobolev can vouch for the fact that these dependencies aren't evil 😈

Opened PR: Add django-stubs, djangorestframework-stubs to allowlist typeshed-internal/stub_uploader#111
stubtest gets exception django.core.exceptions.ImproperlyConfigured from poking around in some django-filter internals. I could just allowlist these.

But if we could somehow set DJANGO_SETTINGS_MODULE env 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 Any for incomplete types. Should I convert all of these to _typeshed.Incomplete or not bother with this?

intgr

Some further questions. Personally I'd prefer to get this merged first and then do further improvements in follow-up PRs.

intgr · 2023-10-22T10:53:45Z

stubs/django-filter/django_filters/fields.pyi

+from django import forms
+
+from .conf import settings as settings
+from .constants import EMPTY_VALUES as EMPTY_VALUES


There are lots of re-exports that shouldn't be re-exported. Should I clean these up before initial merge, or in follow-up?

follow-up is fine imo

intgr · 2023-10-22T10:54:07Z

stubs/django-filter/django_filters/fields.pyi

+    def __init__(self, *args: Any, **kwargs: Any) -> None: ...
+
+class TimeRangeField(RangeField):
+    widget: Any = ...


Should I clean up attribute assignments?

Suggested change

widget: Any = ...

widget: Any

Fine to do this in a follow-up imo

stubs/django-filter/django_filters/rest_framework/backends.pyi

AlexWaygood · 2023-10-22T11:11:19Z

(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

intgr · 2023-10-22T11:30:06Z

For your question (1) [...] unfortunately adding django-stubs and djangorestframework-stubs to 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 typeshed-internal/stub_uploader#90 (comment)

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.

intgr · 2023-10-23T16:02:41Z

I think anything else we can fix/improve afterwards, but the stub_uploader error is a blocker: #10919 (comment)

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?

AlexWaygood · 2023-10-25T17:08:20Z

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?

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.

intgr · 2024-04-09T15:33:10Z

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.

srittau · 2024-07-15T14:05:22Z

@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.

intgr · 2024-07-15T14:11:58Z

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 typeshed-internal/stub_uploader#111 (comment) / typeshed-internal/stub_uploader#90 (comment)

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.

Ganji00 · 2024-07-26T13:55:17Z

@intgr maybe we can try try to make a PR with django-stubs as a dependency to django-filter?

JelleZijlstra · 2024-10-02T00:59:25Z

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?)

hamdanal · 2024-12-22T17:26:05Z

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 typeshed-internal/stub_uploader#152. Perhaps someone can merge main to trigger ci to see if it works.

hamdanal · 2024-12-22T18:06:53Z

stubs/django-filter/METADATA.toml

@@ -0,0 +1,3 @@
+version = "23.3.*"
+upstream_repository = "https://github.com/carltongibson/django-filter"
+requires = ["django-stubs", "djangorestframework-stubs"]


You'd need to remove this to pass the stub_uploader CI. It doesn't look like it is used anywhere in the stubs, not sure why it was included.

Suggested change

requires = ["django-stubs", "djangorestframework-stubs"]

requires = ["django-stubs"]

It was used in __init__.pyi: from . import rest_framework as rest_framework. But indeed this is totally unnecessary and is the only usage, removed.

By the way, thanks a lot for improving stub-uploader, and for letting me know about the changes!

It was used in __init__.pyi: from . import rest_framework as rest_framework. But indeed this is totally unnecessary and is the only usage, removed.

This looks like a locally defined module named rest_framework, not the djangorestframework package. I don't think it should be removed.

By the way, thanks a lot for improving stub-uploader, and for letting me know about the changes!

Thank you and thanks for working on django stubs!

This looks like a locally defined module named rest_framework

Ah right you are!

I don't think it should be removed.

I don't think it does anything. It's re-exporting the module django_filter.rest_framework in django_filter/__init__.py that is already available under the exact same module path? 😄

I don't think it does anything. It's re-exporting the module django_filter.rest_framework in django_filter/__init__.py that is already available under the exact same module path? 😄

If you remove it then this will not work (at least with pyright):

import django_filters django_filters.rest_framework # <- this will be a type error and would not have autocompletion in VS Code

@intgr I still think the the removed from . import rest_framework as rest_framework must be restored. It is deliberately imported at runtime for the specific reason that would let pyright accept the code snippet I mentioned above, i.e. to avoid having to type import django_fiters.rest_framework.

See the comment here:
https://github.com/carltongibson/django-filter/blob/2494df96c6387a9fa411fcb00b696b15dfd9216b/django_filters/__init__.py#L7-L10

I think it's silly and nobody should use it like that. The documentation and tests don't use it that way either.

But if you feel strongly about this, fair enough, I've restored it.

intgr · 2024-12-22T18:20:45Z

It's a mystery to me, why Pyright fails on this file (only with older Pythons)... I didn't even touch it 😄

/home/runner/work/typeshed/typeshed/stubs/setuptools/setuptools/compat/py310.pyi:9:81 - error: Unnecessary "# pyright: ignore" rule: "reportMissingImports" (reportUnnecessaryTypeIgnoreComment)

AlexWaygood · 2024-12-22T18:53:33Z

It's a mystery to me, why Pyright fails on this file (only with older Pythons)... I didn't even touch it 😄

#13101 (comment)

AlexWaygood · 2024-12-22T18:55:14Z

See also my suggested solution for the pyright issue: #13101 (comment)

(But it should be done in a separate PR to this one)

hamdanal · 2024-12-22T19:04:03Z

See also my suggested solution for the pyright issue: #13101 (comment)

(But it should be done in a separate PR to this one)

#13280

…work module

intgr · 2024-12-22T20:21:39Z

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.

github-actions · 2024-12-22T20:42:06Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

This comment has been minimized.

Sign in to view

intgr force-pushed the add-types-django-filter branch from ecdac04 to 1e34eaa Compare October 22, 2023 10:13

This comment has been minimized.

Sign in to view

intgr force-pushed the add-types-django-filter branch from 9cf4747 to 1549268 Compare October 22, 2023 10:31

intgr marked this pull request as ready for review October 22, 2023 10:32

intgr force-pushed the add-types-django-filter branch from 6b08536 to 63e9ab6 Compare October 22, 2023 10:34

This comment has been minimized.

Sign in to view

intgr mentioned this pull request Oct 22, 2023

Add django-stubs, djangorestframework-stubs to allowlist typeshed-internal/stub_uploader#111

Merged

intgr commented Oct 22, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

intgr force-pushed the add-types-django-filter branch from 0bc7b19 to 63e9ab6 Compare October 22, 2023 12:45

This comment has been minimized.

Sign in to view

Viicos mentioned this pull request Apr 15, 2024

Add django-stubs to allowlist typeshed-internal/stub_uploader#135

Closed

This comment has been minimized.

Sign in to view

intgr force-pushed the add-types-django-filter branch from 8955f89 to 4e6227a Compare December 22, 2024 18:02

hamdanal reviewed Dec 22, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

intgr force-pushed the add-types-django-filter branch from 4e6227a to dfe1fbe Compare December 22, 2024 18:14

This comment has been minimized.

Sign in to view

hamdanal mentioned this pull request Dec 22, 2024

setuptools: Fix unused pyright: ignore #13279

Closed

hamdanal mentioned this pull request Dec 22, 2024

setuptools: Remove problematic private module compat #13280

Merged

DavisRayM and others added 5 commits December 22, 2024 22:10

[django-filter] Setup initial structure

58778b5

[django-filter] Fix widget error

eb34321

[django-filter] Explicitly export all the filters from the rest_frame…

9428022

…work module

[django-filter] Update to typeshed style

862b49a

[django-filter] Add django-filter metadata

8956aaf

intgr force-pushed the add-types-django-filter branch from dfe1fbe to db7df55 Compare December 22, 2024 20:10

This comment has been minimized.

Sign in to view

[django-filter] Various minor fixes

4f00327

intgr force-pushed the add-types-django-filter branch from db7df55 to 4f00327 Compare December 22, 2024 20:35

	requires = ["django-stubs", "djangorestframework-stubs"]
	requires = ["django-stubs"]

[django-filter] Absorb django-filter-stubs into typeshed #10919

Are you sure you want to change the base?

[django-filter] Absorb django-filter-stubs into typeshed #10919

Conversation

intgr commented Oct 20, 2023

AlexWaygood commented Oct 20, 2023

This comment has been minimized.

This comment has been minimized.

intgr commented Oct 22, 2023 • edited Loading

This comment has been minimized.

intgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood commented Oct 22, 2023

intgr commented Oct 22, 2023 • edited Loading

This comment has been minimized.

This comment has been minimized.

intgr commented Oct 23, 2023 • edited Loading

AlexWaygood commented Oct 25, 2023

intgr commented Apr 9, 2024 • edited Loading

This comment has been minimized.

srittau commented Jul 15, 2024

intgr commented Jul 15, 2024 • edited Loading

Ganji00 commented Jul 26, 2024

JelleZijlstra commented Oct 2, 2024

This comment has been minimized.

hamdanal commented Dec 22, 2024

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intgr Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

hamdanal Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

intgr Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

intgr commented Dec 22, 2024

This comment has been minimized.

AlexWaygood commented Dec 22, 2024

AlexWaygood commented Dec 22, 2024

hamdanal commented Dec 22, 2024

This comment has been minimized.

intgr commented Dec 22, 2024

github-actions bot commented Dec 22, 2024

intgr commented Oct 22, 2023 •

edited

Loading

intgr commented Oct 22, 2023 •

edited

Loading

intgr commented Oct 23, 2023 •

edited

Loading

intgr commented Apr 9, 2024 •

edited

Loading

intgr commented Jul 15, 2024 •

edited

Loading

intgr Dec 22, 2024 •

edited

Loading

hamdanal Dec 22, 2024 •

edited

Loading

intgr Dec 22, 2024 •

edited

Loading