Opened 8 years ago

Last modified 2 days ago

#27880 new Cleanup/optimization

Use __set_name__ to replace some usages of contribute_to_class.

Reported by: Simon Charette Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: python3.6
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If we were to define __set__name__ calling contribute_to_class() on models.Field, Manager and Options the following things could be done:

  1. ModelBase.add_to_class calls could be replaced by simple setattr(cls, name, value).
  2. Options.contribute_to_class could be inlined in Options.__set_name__.
  3. Manager.contribute_to_class could be inlined in Manager.__set_name__.
  4. There might be a way to inline Field.contribute_to_class by dealing with its private_only flag somehow.

Change History (10)

comment:1 by Mariusz Felisiak, 4 years ago

Triage Stage: Someday/MaybeAccepted

comment:2 by Clifford Gama, 4 months ago

Owner: changed from nobody to Clifford Gama
Status: newassigned

comment:3 by Clifford Gama, 4 months ago

Has patch: set
Needs documentation: set
Needs tests: set

comment:5 by Clifford Gama, 3 months ago

Closed https://github.com/django/django/pull/18621 as the work on Field.contribute_to_class() was not production-ready, and as there was no feedback on the implementation in forum discussion.

comment:6 by Clifford Gama, 3 months ago

PR addressing this ticket for Options

comment:7 by Clifford Gama, 3 months ago

Note that while Options.contribute_to_class() is private API, searching /(?-i)Options/ django.db "def contribute_to_class" -path:django/db path:*py in GitHub gives about 240 matches, some of which implement their own Options. These would need to rename contribute_to_class() with __set_name__() as this change is backward incompatible.

Last edited 3 months ago by Clifford Gama (previous) (diff)

comment:8 by Matthias Kestenholz, 3 days ago

It's not that easy to write library code which supports both code paths. I "complained" to Loïc at DutH 2015 (or something) about the get_query_set -> get_queryset rename. That one was really hard for third party app maintainers to work around and support all possible combinations given that people are supposed to use subclassing to customize behaviors, even when I agreed with the rename on the basis that get_queryset reads better and is more consistent. (That's to say it was a good change.)

This seems to be a similar case. It's nice that Python now supports calling this method and that we could in principle use it for our own purposes, but the explicitness of contribute_to_class might still have some value. Especially given the fact that the change doesn't allow us to remove a lot of code in the future, and given that it replaces an explicit mechanism with a more implicit mechanism where people have to know more of Python's internals to do what they want.

Also, you write that contribute_to_class is private API. It *is* mentioned in the docs/ folder of the Django repository so I'm not sure I agree with that.

I'm quite sure the potential downsides of this change, especially the impact to third party app developers are bigger than the upsides of using new standard Python API.

Last edited 3 days ago by Matthias Kestenholz (previous) (diff)

comment:9 by Simon Charette, 3 days ago

Thanks for chiming in Matthias!

I'll note that initial proposal of this ticket was not about replacing/deprecating contribute_to_class which would effectively be very disruptive to the ecosystem but to remove the ModelBase.__new__ magic that performs explicit calls to contribute_to_class by having __set_name__ call contribute_to_class instead.

To me this ticket kind of go hand-in-hand with __init_subclasses__ tickets #35827 and #34555 in the sense that it would be great to modernize the implementation of ModelBase.__new__ in a way that takes advantage of these hooks while maintaining the developer facing interface and behavior. It's easier said than done though and not a well understood problem from a feasibility perspective.

comment:10 by Clifford Gama, 2 days ago

Owner: Clifford Gama removed
Status: assignednew

Thank you for clarifying the situation.

I'm not very experienced with the ecosystem, so I kind of suspected that I'd done some (many) things wrong and was waiting for feedback on this. So thanks for the feedback.

I don't think this is something I can take on, so I'm de-assigning myself and closing related PRs. Great learning experience, though.

Note: See TracTickets for help on using tickets.
Back to Top