Skip to content

[django-filter] Improve constructor param types and add missing __all__ #14556

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Aug 10, 2025

This is all I found that could be improved in #14540. FYI @huynguyengl99

  • Added __init__ params that are inherited from parent classes. Reduced usage of loosely typed *args, **kwargs.
  • Removed __init__ method type hints from classes whose parameters are same as parent class -- to avoid duplicating them.
  • Added __all__ to django_filters/rest_framework/__init__.pyi -- the imports in this file are clearly meant as re-export, but without __all__ I believe type-checkers don't allow using them.

Copy link
Contributor

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

@donBarbos
Copy link
Contributor

Just a quick note — it’s generally a good practice to add a comment for each use of Any. If you’re unsure whether it should be Any, it might be better to use Incomplete instead.
It’s also important to ensure that the stubtests are passing. And regarding __all__ in type stubs - it should simply reflect the real __all__ in the source code, and it’s worth including it in the stubs if it exists in the source.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but one question below.

Comment on lines +23 to +28
# Based on django-stubs utils/choices.pyi
_Choice: TypeAlias = tuple[Any, Any]
_ChoiceNamedGroup: TypeAlias = tuple[str, Iterable[_Choice]]
_Choices: TypeAlias = Iterable[_Choice | _ChoiceNamedGroup]
_ChoicesMapping: TypeAlias = Mapping[Any, Any]
_ChoicesInput: TypeAlias = _Choices | _ChoicesMapping | type[Choices] | Callable[[], _Choices | _ChoicesMapping]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that we require django-stubs, can't we just import those from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, but IIRC received a mypy error about importing non-public members. Will try again and report back.

@intgr
Copy link
Contributor Author

intgr commented Aug 11, 2025

Thanks, I'll find the time some day to fix CI errors and resolve review comments.

__all__ in type stubs - it should simply reflect the real __all__ in the source code

👍 I will convert them to import X as X imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants