Hi Russell, Many thanks to Daniel, and to you, for all the work on this PR!
On 12/22/2014 12:39 AM, Russell Keith-Magee wrote: > * get_fields(include_parents=True, include_hidden=False) > > Returns a list of fields associated with a model. > > include_parents controls whether the list of fields is those defined > literally on this model, or if fields on the parent model are also included > (for inheritance situations). > > include_hidden controls whether hidden fields are returned. Hidden fields > are fields that exist for internal purposes, but aren't part of public API, > or have been explicitly hidden by a user. For example, the reverse relation > associated with a ForeignKey with a related_name that starts with "+" is a > hidden field - the reverse relation exists, but is hidden from public use. One thing I've mentioned on the PR is that currently `get_fields` also accepts `**kwargs`, which is used for some internal-only flags that are used only when it recursively calls itself. I would prefer to see this cleaned up so the API of the public method is precisely what is documented, and no more (accepting `**kwargs` means that an end-user typo of an argument name could result in silent wrong behavior rather than an argument error), and if internal-only recursive behavior is needed, use a separate internal method (called by the public method) for that. > * Handling of Generic Foreign Keys (esp in admin) > > In the old API, get_fields() didn't return GenericForeignKeys. The new API > does, but they can be filtered out. > > The only place this really matters in Django itself is in contrib.admin, > which can't display widgets for GFKs; as a result, they need to be manually > excluded from field lists. > > This is currently being done with an internal method named > `_get_non_gfk_field()`. > > In his review notes, Carl commented that we should be avoiding > field-specific names like "GFK", in favour of capturing the underlying > concept. This is a fair point, but leads to the question of what the > "underlying" concept is here. > > At one level, there's a reasonable argument that GFKs can be singled out > here - the use is an internal method, in contrib.admin, and GFKs are the > only field type admin doesn't support out of the box. > > At a higher level, though, a GFK is the only example of a virtual field in > Django - a field. This might change, however - some of the Composite Field > changes, plus possible change to ForeignKey might lead to more "virtual" > fields being introduced - some of which *will* be visible in the admin. > > The question: Do we - > > 1) Accept this particular internal specific naming of GFK as a quirk > reflecting the limitations of contrib.admin > > 2) Try to nail down what a "virtual" field means, or find some alternative > flag to identify what "GFK" is a proxy for in this case. > > In my opinion, (1) is acceptable at the moment; when Composite Keys (and > possibly the ForeignKey refactor) land, we can revisit what virtual means > and possibly address this issue in admin at the same time. It is for this > reason that I don't think virtual has a place in formal documentation for > 1.8, even though the flag exists, and is used by GFKs. The issue that concerned me was not the name `_get_non_gfk_field` in the admin, but the presence of a non-standard `is_gfk` flag on GenericForeignKey, which was special-cased in several places, in `_get_non_gfk_field` but also two places in the core ORM. The `is_gfk` flag has since been removed and replaced by appropriate capability flag checks on the field (in https://github.com/PirosB3/django/commit/3f668766f67669bc9a19ae78e888840609e443c5). So my concern has already been addressed. I don't have a problem with an internal utility function in the admin named `_get_non_gfk_field`. It's helpful to know what the current thinking on `virtual` is. It initially seemed odd to me that this PR, whose goal was to clean up and standardize field categories/flags, didn't define or make any use of `virtual`, but also didn't remove it. From what you've said here, it seems the thinking is "it may be useful in future but we don't have enough examples yet to know how it should be used, so we'll keep it around but not make it public yet." That seems OK to me. It might be nice if that thinking were recorded in a code comment or docstring somewhere. > * The "model", "parent_model" and "related_model" properties on fields. > > The new API ensures that every field has a `model` attribute, describing > the model that owns/defines the field. > > However, there's a backwards compatibility problem. RelatedObject - the > object used to represent the "other" side of FK and M2M relations - is also > returned by get_fields(), and *it* uses the '.model' attribute to store the > model that defines the field that the RelatedObject is managing. So - if > Book has a foreign key to Author, Author will have a RelatedObject named > book_set; and book_set.model will be Book. > > Related Objects also have a "parent_model" attribute that identifies the > other end of the relation - the model that is being pointed *at*. > > As part of this refactor, relation fields like ForeignKey have aquired a > "related_model" attribute that - so given a ForeignKey, you can tell what > model it is pointing at. > > This means there's an inconsistency here - if you think of RelatedObject as > being a "reverse" ForeignKey/M2M, then the model should be Author, and > related_model should be Book. The name "parent_model" doesn't make a whole > lot of sense in this context > > The question: Do we: > > 1) Accept this as a backwards incompatible change. > > 2) Accept the inconsistency in the new API. > > 3) Find a pair of names other than model/related_model to represent the > "subject/object" pair in fields > > My inclination is for (1). RelatedObject isn't stable API at present; the > whole purpose of formalising _meta is to formalise some of these historical > inconsistencies. > > RelatedObject is an internal implementation detail, and one that's only > visible if you're using the old "get_all_related_objects()" (and similar) > API. > > I think the inconvenience caused by making this change is worth it. The new > naming is internally consistent, and much less surprising; the set of > people who will be affected by this change is also the set of people who > are already spelunking through _meta; it's highly unlikely they're going to > have a completely painless 1.8 transition. I fully agree: if the idea is to clean up, let's clean up! To be clear, does that mean getting rid of the `parent_model` attribute entirely, in favor of a standardized `model` and `related_model`? That's what I would favor. The current code in the PR actually expands the use of `parent_model` so that it is always set on all fields (and in the case of non-related fields, is the same as `model`). I think this is not a good idea, for two reasons: 1) As you noted, `parent_model` is a confusing name in this context. `related_model` is a much better name. 2) For a non-has-relation field, it doesn't make sense for `parent/related_model` to be set to the same as `model` - it would make more sense for it to be `None`. > Summary > ======= > > I think PR 3114 is pretty close to ready; I've highlighted my preferred > options for the two outstanding design issues, both of which are pretty > easy to resolve once a decision is made. A few other small issues that I would still like to see resolved: 1) Currently FieldFlagsMixin has a number of dynamic properties that are only dynamic because they special-case checks for `is_reverse_object` -- I think it would be better if whatever sets `is_reverse_object` (only `RelatedObject` I think?) simply switched the appropriate flags itself (replacing a series of conditionals with polymorphic behavior). 2) Currently `FieldFlagsMixin` does not only contain flags, it also contains a definition of the `related_model` property. Perhaps it should be renamed to simply `FieldMixin` if it is not limited to flags? 3) I think it would be better if all the cardinality flags were present on the base `FieldFlagsMixin` and defaulted to `None`. That would permit many checks for e.g. `if field.has_relation and field.one_to_many` to be safely replaced with the more concise `if field.one_to_many`. In general I think `None` is a better choice than `raise AttributeError` for standard flags that don't apply to a particular field. 4) There is an open PR (originally authored by Anssi, recently updated by Tim) to remove RelatedObject entirely: https://github.com/django/django/pull/3757 -- I think that's a good PR and should be merged for 1.8, but it will require some non-trivial updates to the _meta PR once it is merged. Carl Carl -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/54987C86.40801%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: OpenPGP digital signature
