-
-
Notifications
You must be signed in to change notification settings - Fork 561
Hygienic mypy plugin Phase 0: route direct Django access through DjangoContext #3376
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,14 +116,117 @@ def __init__(self, django_settings_module: str) -> None: | |
| self.django_settings_module = django_settings_module | ||
|
|
||
| apps, settings = initialize_django(self.django_settings_module) | ||
| self.apps_registry = apps | ||
| self.settings = settings | ||
| self._apps_registry = apps | ||
| self._settings = settings | ||
|
|
||
| # ---- Registry wrappers ---- | ||
|
|
||
| def get_model_class_by_label(self, label: str) -> type[Model] | None: | ||
| try: | ||
| return self._apps_registry.get_model(label) | ||
| except LookupError: | ||
| return None | ||
|
|
||
| def is_app_installed(self, dotted_or_label: str) -> bool: | ||
| return self._apps_registry.is_installed(dotted_or_label) | ||
|
|
||
| # ---- Settings wrappers ---- | ||
|
|
||
| def get_setting(self, name: str, default: Any = None) -> Any: | ||
| return getattr(self._settings, name, default) | ||
|
|
||
| def has_setting(self, name: str) -> bool: | ||
| return hasattr(self._settings, name) | ||
|
|
||
| @cached_property | ||
| def auth_user_model_label(self) -> str: | ||
| return self._settings.AUTH_USER_MODEL | ||
|
|
||
| @cached_property | ||
| def default_auto_field_path(self) -> str: | ||
| return self._settings.DEFAULT_AUTO_FIELD | ||
|
|
||
| # ---- Single-model meta wrappers ---- | ||
|
|
||
| def get_field_on_model(self, model_cls: type[Model], field_name: str) -> Field[Any, Any] | ForeignObjectRel | None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. naming is not super consistant, Again this is a bit confusing on usage, maybe use somthing like:
|
||
| try: | ||
| return model_cls._meta.get_field(field_name) | ||
| except FieldDoesNotExist: | ||
| return None | ||
|
|
||
| def is_model_abstract(self, model_cls: type[Model]) -> bool: | ||
| return model_cls._meta.abstract | ||
|
Comment on lines
+157
to
+158
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A lot of these are static (they don't use self) maybe add You can enable the ruff rule
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's please model our APIs without
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with @sobolevn here, the fact that they don't use self is just accidental here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But they will never need self, they don't need coupling with django_context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not if all they do is access
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main point of this phase is to surface what queries we are asking from the Django runtime and consolidating them in a single place so we can design a better abstraction. |
||
|
|
||
| def get_proxy_target(self, model_cls: type[Model]) -> type[Model] | None: | ||
| return model_cls._meta.proxy_for_model | ||
|
|
||
| def get_auto_field(self, model_cls: type[Model]) -> Field[Any, Any] | None: | ||
| return model_cls._meta.auto_field | ||
|
|
||
| def get_pk_field_or_none(self, model_cls: type[Model]) -> Field[Any, Any] | None: | ||
| # Non-raising parallel of `get_primary_key_field`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not correct, The typing also suggest this can only return Field, never None class Options(Generic[_M]):
pk: FieldWe should probably investigate django runtime for this and remove one of the two helpers |
||
| return model_cls._meta.pk | ||
|
|
||
| def get_pk_fields(self, model_cls: type[Model]) -> tuple[Field[Any, Any], ...]: | ||
| # Composite-PK ready (Django 5.2+); falls back to the single PK on older Django. | ||
| pk_fields = getattr(model_cls._meta, "pk_fields", None) | ||
| if pk_fields is not None: | ||
| return tuple(pk_fields) | ||
| pk = self.get_pk_field_or_none(model_cls) | ||
| return (pk,) if pk is not None else () | ||
|
|
||
| def get_model_parents(self, model_cls: type[Model]) -> Sequence[type[Model]]: | ||
| # Wraps `_meta.all_parents` (Django 5.2+) / `_meta.get_parent_list()` for older Django. | ||
| opts = model_cls._meta | ||
| parents = getattr(opts, "all_parents", None) | ||
| if parents is not None: | ||
| return list(parents) | ||
| return opts.get_parent_list() | ||
|
|
||
| def iter_reverse_relations(self, model_cls: type[Model]) -> Iterator[ForeignObjectRel]: | ||
| # Public-API equivalent of `_meta.related_objects` (private API). | ||
| # Matches `include_hidden=True` to preserve behaviour for hidden M2M etc. | ||
| for field in model_cls._meta.get_fields(include_hidden=True): | ||
| if isinstance(field, ForeignObjectRel): | ||
| yield field | ||
|
Comment on lines
+186
to
+191
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation changed, is it expected ? |
||
|
|
||
| # ---- Manager wrappers ---- | ||
|
|
||
| def iter_managers(self, model_cls: type[Model]) -> Iterator[tuple[str, models.Manager[Any]]]: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design question: is it fine for a separate mypy plugin to return
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most likely not, but the intent of this phase is to make those leaks more visible via the single |
||
| yield from model_cls._meta.managers_map.items() | ||
|
|
||
| def get_default_manager_class(self, model_cls: type[Model]) -> type[models.Manager[Any]]: | ||
| return model_cls._meta.default_manager.__class__ # type: ignore[return-value] | ||
|
|
||
| # ---- Query helpers ---- | ||
|
|
||
| def resolve_names_to_path(self, model_cls: type[Model], parts: Sequence[str]) -> tuple[_AnyField, Sequence[str]]: | ||
| """Wrap `Query(model).names_to_path(parts, _meta)` so callers don't touch `_meta`. | ||
|
|
||
| Returns `(final_field, remainder)`. Raises Django's `FieldError` for the caller | ||
| to translate into a user-facing diagnostic. | ||
| """ | ||
| _, final_field, _, remainder = Query(model_cls).names_to_path(parts, model_cls._meta) | ||
| return final_field, remainder | ||
|
|
||
| def iter_select_related_choices(self, model_cls: type[Model]) -> Iterator[str]: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only use it in What about just having a |
||
| """Names valid for `QuerySet.select_related(...)` on this model. | ||
|
|
||
| Mirrors Django's `SQLCompiler.get_related_selections._get_field_choices`: | ||
| forward FK/O2O field names plus reverse-unique relation query names. | ||
| """ | ||
| for field in model_cls._meta.fields: | ||
| if field.is_relation: | ||
| yield field.name | ||
| for rel in model_cls._meta.related_objects: | ||
| if rel.field.unique: | ||
| yield rel.field.related_query_name() | ||
|
|
||
| @cached_property | ||
| def model_modules(self) -> dict[str, dict[str, type[Model]]]: | ||
| """All modules that contain Django models.""" | ||
| modules: dict[str, dict[str, type[Model]]] = defaultdict(dict) | ||
| for concrete_model_cls in self.apps_registry.get_models(include_auto_created=True, include_swapped=True): | ||
| for concrete_model_cls in self._apps_registry.get_models(include_auto_created=True, include_swapped=True): | ||
| modules[concrete_model_cls.__module__][concrete_model_cls.__name__] = concrete_model_cls | ||
| # collect abstract=True models | ||
| for model_cls in concrete_model_cls.mro()[1:]: | ||
|
|
@@ -197,7 +300,7 @@ def get_primary_key_field(self, model_cls: type[Model]) -> Field[Any, Any]: | |
| raise ValueError("No primary key defined") | ||
|
|
||
| def get_expected_types(self, api: TypeChecker, model_cls: type[Model], *, method: str) -> dict[str, MypyType]: | ||
| contenttypes_in_apps = self.apps_registry.is_installed("django.contrib.contenttypes") | ||
| contenttypes_in_apps = self._apps_registry.is_installed("django.contrib.contenttypes") | ||
|
|
||
| expected_types = {} | ||
| # add pk if not abstract=True | ||
|
|
@@ -272,7 +375,7 @@ def get_expected_types(self, api: TypeChecker, model_cls: type[Model], *, method | |
|
|
||
| @cached_property | ||
| def all_registered_model_classes(self) -> set[type[models.Model]]: | ||
| model_classes = self.apps_registry.get_models() | ||
| model_classes = self._apps_registry.get_models() | ||
|
|
||
| all_model_bases = set() | ||
| for model_cls in model_classes: | ||
|
|
@@ -387,7 +490,7 @@ def get_field_related_model_cls(self, field: RelatedField[Any, Any] | ForeignObj | |
| raise UnregisteredModelError | ||
| else: | ||
| try: | ||
| related_model_cls = self.apps_registry.get_model(related_model_cls) | ||
| related_model_cls = self._apps_registry.get_model(related_model_cls) | ||
| except LookupError as e: | ||
| raise UnregisteredModelError from e | ||
|
|
||
|
|
@@ -577,4 +680,4 @@ def resolve_f_expression_type(self, f_expression_type: Instance) -> ProperType: | |
|
|
||
| @cached_property | ||
| def is_contrib_auth_installed(self) -> bool: | ||
| return "django.contrib.auth" in self.settings.INSTALLED_APPS | ||
| return "django.contrib.auth" in self._settings.INSTALLED_APPS | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -606,8 +606,8 @@ def resolve_string_attribute_value(attr_expr: Expression, django_context: Django | |||||||||
| if isinstance(attr_expr, MemberExpr): | ||||||||||
| member_name = attr_expr.name | ||||||||||
| if isinstance(attr_expr.expr, NameExpr) and attr_expr.expr.fullname == "django.conf.settings": | ||||||||||
| if hasattr(django_context.settings, member_name): | ||||||||||
| return getattr(django_context.settings, member_name) # type: ignore[no-any-return] | ||||||||||
| if django_context.has_setting(member_name): | ||||||||||
| return django_context.get_setting(member_name) # type: ignore[no-any-return] | ||||||||||
|
Comment on lines
+609
to
+610
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
| return None | ||||||||||
|
|
||||||||||
|
|
||||||||||
|
|
@@ -736,7 +736,7 @@ def get_model_from_expression( | |||||||||
| and isinstance(expr.expr, NameExpr) | ||||||||||
| and f"{expr.expr.fullname}.{expr.name}" == fullnames.AUTH_USER_MODEL_FULLNAME | ||||||||||
| ): | ||||||||||
| lazy_reference = django_context.settings.AUTH_USER_MODEL | ||||||||||
| lazy_reference = django_context.auth_user_model_label | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels weird to me to stuff everything on |
||||||||||
|
|
||||||||||
| if lazy_reference is not None: | ||||||||||
| model_info = resolve_lazy_reference(lazy_reference, api=api, django_context=django_context, ctx=expr) | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd:
cached_property(they did not have any before)get_settingshelperI think we should remove the
cached_property, looks like premature optimization.If settings lookup are expensive, then we should be doing caching at the
get_settinglayer, and if they are not, we should not do caching