Skip to content

[django-filter] Improve constructor param types and add missing re-exports #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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 14, 2025
Merged

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

merged 1 commit into from
Aug 14, 2025

Conversation

intgr
Copy link
Contributor

@intgr intgr commented Aug 10, 2025 •
edited
Loading

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

  • Accept StrOrPromise for field labels -- allow Django lazy translated strings.
  • 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 missing re-exports to django_filters/rest_framework/__init__.pyi -- the imports in this file are clearly meant as re-export

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@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.

srittau
srittau reviewed Aug 11, 2025
View reviewed changes
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.

@intgr
Copy link
Contributor Author

intgr commented Aug 11, 2025 •
edited
Loading

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.

@intgr intgr changed the title [django-filter] Improve constructor param types and add missing __all__ [django-filter] Improve constructor param types and add missing re-exports Aug 12, 2025
@intgr intgr force-pushed the improve-django-filter branch from 63a8427 to 5ca265b Compare August 12, 2025 21:05
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@intgr
[django-filter] Improve constructor param types and add missing re-ex…
014fff7 
…ports

* Accept `StrOrPromise` for field labels -- allow Django lazy translated strings.
* 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 missing re-exports to `django_filters/rest_framework/__init__.pyi` -- the imports in this file are clearly meant as re-export
@intgr intgr force-pushed the improve-django-filter branch from 5ca265b to 014fff7 Compare August 14, 2025 17:26
@github-actions GitHub Actions
Copy link
Contributor

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

@intgr
Copy link
Contributor Author

intgr commented Aug 14, 2025

Tests fixed -- restored the _ChoicesInput I copied from django-stubs (and other aliases referenced by _ChoicesInput)

@srittau srittau merged commit 893b9a7 into python:main Aug 14, 2025
48 checks passed
@intgr intgr deleted the improve-django-filter branch August 14, 2025 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srittau srittau srittau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@intgr @donBarbos @srittau