Skip to content
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

Add django.db.models.fields.related.ForeignObject missing methods and attributes #2154

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JaeHyuckSa
Copy link
Contributor

I have made things!

Some of the Django 5.0 updates were missing from the related classes, so I added them referring to the allowlist_todo.

If you need any additional work, please let me know and I'll work on it right away. Thank you :)

django.db.models.ForeignObject

  • django.db.models.ForeignObject.__copy__
  • django.db.models.ForeignObject.contribute_to_related_class
  • django.db.models.ForeignObject.forward_related_accessor_class
  • django.db.models.ForeignObject.get_class_lookups
  • django.db.models.ForeignObject.get_extra_descriptor_filter
  • django.db.models.ForeignObject.get_foreign_related_value
  • django.db.models.ForeignObject.get_instance_value_for_fields
  • django.db.models.ForeignObject.get_joining_columns
  • django.db.models.ForeignObject.get_local_related_value
  • django.db.models.ForeignObject.get_path_info
  • django.db.models.ForeignObject.get_reverse_joining_columns
  • django.db.models.ForeignObject.get_reverse_path_info
  • django.db.models.ForeignObject.related_accessor_class
  • django.db.models.ForeignObject.requires_unique_target

Related issues

@@ -87,6 +94,9 @@ class RelatedField(FieldCacheMixin, Field[_ST, _GT]):

class ForeignObject(RelatedField[_ST, _GT]):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this class has lots of unsolved problems. Let's solve them!

  1. Please, use ClassVar where needed: https://github.com/django/django/blob/a09082a9bec18f8e3ee8c10d473013ec67ffe93b/django/db/models/fields/related.py#L522-L530

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. As you suggested, I have handled all the relevant parts as class variables.

django.db.models.fields.related.ForeignObject.path_infos
django.db.models.fields.related.ForeignObject.related_accessor_class
django.db.models.fields.related.ForeignObject.requires_unique_target
django.db.models.fields.related.ForeignObject.get_extra_restriction
Copy link
Member

Choose a reason for hiding this comment

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

  1. Let's add other missing attributes to ForeignObject

Copy link
Contributor Author

@JaeHyuckSa JaeHyuckSa May 16, 2024

Choose a reason for hiding this comment

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

I have two questions:

  1. I tried to define the get_extra_restriction method, but I encountered an error due to the parameter mismatch with the subclass. I attempted to use type: ignore on the method in the subclass, but it was not allowed. What steps can I take to resolve this issue?
error: not checking stubs due to mypy build errors:
django-stubs/contrib/contenttypes/fields.pyi:90: error: Signature of "get_extra_restriction" incompatible with supertype "ForeignObject"  [override]
django-stubs/contrib/contenttypes/fields.pyi:90: note:      Superclass:
django-stubs/contrib/contenttypes/fields.pyi:90: note:          def get_extra_restriction(self, alias: str, related_alias: str) -> None
django-stubs/contrib/contenttypes/fields.pyi:90: note:      Subclass:
django-stubs/contrib/contenttypes/fields.pyi:90: note:          def get_extra_restriction(self, where_class: Type[WhereNode], alias: Optional[str], remote_alias: str) -> WhereNode
django-stubs/contrib/contenttypes/fields.pyi:92: error: Unused "type: ignore" comment  [unused-ignore]
  1. Should I also add the cached_property method?

@@ -124,6 +134,7 @@ class ForeignObject(RelatedField[_ST, _GT]):
error_messages: _ErrorMessagesMapping | None = ...,
db_comment: str | None = ...,
) -> None: ...
def __copy__(self) -> Empty: ...
Copy link
Collaborator

@intgr intgr May 13, 2024

Choose a reason for hiding this comment

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

I think this should actually be Self

The Empty class is just an artifact of a weird hack that Django uses, but it modifies the __class__ attribute so that it's no longer actually the Empty class

    def __copy__(self):
        # We need to avoid hitting __reduce__, so define this
        # slightly weird copy construct.
        obj = Empty()
        obj.__class__ = self.__class__
        obj.__dict__ = self.__dict__.copy()
        return obj

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on your explanation, that seems correct. Thank you for pointing that out!

@intgr intgr changed the title Add typing of django.db.models.fields.related.ForeignObject Add django.db.models.fields.related.ForeignObject missing methods and attributes May 13, 2024
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.

3 participants