On 12/22/2014 06:04 PM, Russell Keith-Magee wrote: [snipped agreement on many things!] > I'd argue whether a mixin is needed at all. At present, the mixin is only > used on Field, GenericForeignKey and RelatedObject. RelatedObject will > potentially be deleted; that leaves 2 classes. Every field in Django > *except* for GenericForeignKey extends Field, so just having the attributes > on Field with default values (just like all the other field attributes), > and duplicating their definition on GenericForeignKey seems like a better > approach to me. > > The only reasons to have a mixin or base class is if (a) there's complex > logic, and/or (b) we have reason to believe that the functionality will be > used elsewhere. If it's just a set of flags, then (a) isn't true; and I'm > having difficulty thinking of any circumstance where (b) would be true, > either.
This seems fine to me - at the moment it's not entirely clear to me what determines whether something belongs on `FieldFlagsMixin` vs `Field`. The one other reason I could think of to keep them separate is if we want a smaller base class "documenting" (in code) precisely the required API for something that wants to be a meta-API compatible field, without any of the Django-ORM-specific stuff in Field (if you were building some kind of non-Django-ORM but admin-compatible thing like Daniel's impressive early Gmail prototype). But if that's what we want out of the separation, I'm not sure we're quite there yet -- e.g. the `name`, `attname`, `model` attributes at least appear to be required for a field to participate in the meta API (at least they seem to be accessed in `Options`), but they aren't currently mentioned on `FieldFlagsMixin`. > 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. >> > > Agreed. Given that our suggested mode of usage is filtering/list > comprehensions, we should make it as easy as possible to implement those. > > 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. > > > So... it's a race to commit, huh :-) Guess so :-) Tim, are you planning to commit that patch or would you like someone else to take a look first? > Seriously - I haven't looked closely at that patch, but the plumbing around > RelatedObect has always been a bit messy; a cleanup would be great. There's > certainly overlap in the two patches, and I agree it would be good to land > both for 1.8 - if the API for related objects is going to change, better to > do it in one pass. Yep, agreed. 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/5498BF33.6000901%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: OpenPGP digital signature
