Skip to content

Add a test for missing generics in stubs #2659

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

Merged
merged 6 commits into from
May 8, 2025

Conversation

UnknownPlatypus
Copy link
Contributor

@UnknownPlatypus UnknownPlatypus commented May 6, 2025

I have made things!

Followup to this discussion #2384 (comment)

This introduce a test ensuring every generic we define in stubs are generic at runtime too.

This also adds the missing ones (SetPasswordForm, discussed above, being one of them)

See initial failing run
FAILED tests/test_generic_consistency.py::test_find_classes_inheriting_from_generic - AssertionError: classproperty is not patched in `ext/django_stubs_ext/patch.py`
  ConnectionProxy is not patched in `ext/django_stubs_ext/patch.py`
  Options is not patched in `ext/django_stubs_ext/patch.py`
  BaseIterable is not patched in `ext/django_stubs_ext/patch.py`
  ReverseOneToOneDescriptor is not patched in `ext/django_stubs_ext/patch.py`
  ForwardManyToOneDescriptor is not patched in `ext/django_stubs_ext/patch.py`
  SetUnusablePasswordMixin is not patched in `ext/django_stubs_ext/patch.py`
  SetPasswordForm(SetPasswordMixin, Form, BaseForm, RenderableFormMixin, RenderableMixin) is not patched in `ext/django_stubs_ext/patch.py`
  AdminPasswordChangeForm(SetUnusablePasswordMixin, SetPasswordMixin, Form, BaseForm, RenderableFormMixin, RenderableMixin) is not patched in `ext/django_stubs_ext/patch.py`
  SetPasswordMixin is not patched in `ext/django_stubs_ext/patch.py`
  ModelFormOptions is not patched in `ext/django_stubs_ext/patch.py`

@UnknownPlatypus UnknownPlatypus marked this pull request as draft May 6, 2025 15:57
Comment on lines 111 to 114
# logger.warning(
# "`SetPasswordMixin` and `SetUnusablePasswordMixin` where not patched, django need to be configured "
# "for this to be possible. Make sure to use `django_stubs_ext.monkeypatch()` after `django.setup()`.",
# )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is annoying, we cannot easily patch these 2 because we end up importing the User model and it crashes if django.setup() was not called beforehand.

The recommendation from the readme to add the monkeypatch in the setting file might not be enough for this.

I commented the logs here for the moment because I'm not sure what is the best way to handle this (and if I add it, I need to find a way to silence this logger in test or it fails some snapshot)

Any idea on how to handle this @sobolevn ? (or @adamchainz maybe if you have some time)

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've documented a workaround for these symbols that seem to work on my django codebases.

If you use these generics, you'll have to rerun the monkeypatch after django is initialized.
We could also maybe expose another monkeypatch func for these

@UnknownPlatypus UnknownPlatypus marked this pull request as ready for review May 7, 2025 14:34
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

How long does it take to run this test in CI? (sorry, not from a work computer right now)

@UnknownPlatypus
Copy link
Contributor Author

How long does it take to run this test in CI? (sorry, not from a work computer right now)

Locally it was ~230ms, ci not sure, I can activate pytest timings to check once I'm back home from my concert X)

@UnknownPlatypus
Copy link
Contributor Author

UnknownPlatypus commented May 8, 2025

How long does it take to run this test in CI? (sorry, not from a work computer right now)

1-1.3s in ci !

python3.10 - 1.10s call     tests/test_generic_consistency.py::test_find_classes_inheriting_from_generic 
python3.11 - 1.03s call     tests/test_generic_consistency.py::test_find_classes_inheriting_from_generic
python3.12 - 1.39s call     tests/test_generic_consistency.py::test_find_classes_inheriting_from_generic
python3.13 - 1.16s call     tests/test_generic_consistency.py::test_find_classes_inheriting_from_generic

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Looks like a great starting point! Thanks a lot! It found many classes that we forgot to monkey patch :)

@sobolevn sobolevn merged commit cab6aeb into typeddjango:master May 8, 2025
37 checks passed
jose-reveni pushed a commit to jose-reveni/django-stubs that referenced this pull request Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants