Hi Thomas, First, thanks for your work. I've had a plan to start working on this feature last week, but I got too many things to work on with higher priority, Thankfully you opted for this, again Thanks.
just sharing my thoughts, 1) I like the MoneyField model style definition. 2) I don't like the inline declaration, I would opt for Anssi's proposal. 3) as Aymeric mentioned, NOT NULL only makes sense for a real column on the database, so that should be only be handled on the SubFields. Assigning python `None' to the CompositeField should be handled by the value_from_dict method. 4) querying syntax, sounds fare. However depending on how we decide on exposing the subfields through the Model Meta API, eg. if the subfields would be returned by default, then we should also allow directly querying of the subfield. Aron On Saturday, March 7, 2015 at 6:31:53 AM UTC-5, Aymeric Augustin wrote: > > Hello Thomas, > > It’s hard for me to digest a two-page-long email on a complex topic during > the week. I bookmarked your first email when it came in. It’s Saturday, > 11am, > and I dedicated my first chunk of quality brain time to reading the entire > thread. I’ll let you ponder what the effect of your second email was. > > In fact, if you propose something stupid, you’ll get a quick answer, > because > it’s easy to explain. Not getting answers right now means that your > proposal > is good enough to require consideration. Yes, that’s counter-intuitive. > > With that out of the way, here are my thoughts on your proposal. Some > overlap > with what other people have said in the thread. > > 1) I would find it reassuring if you described the landscape of past > attempts, > what good ideas you’re keeping, what bad ideas you’re throwing away — in > short, if you convinced us that you’re building on top of past attempts. > The > main goal of this exercise is to guarantee that you don’t overlook a use > case > or an argument that made consensus in the past. (Don’t spend too much time > on > code; it my experience it’s harder to reuse and code matters much less > than > design. > > 2) The syntax for inline composite fields doesn't look very nice. Could > you > simplify it somehow? Anssi’s proposal is good. I assume that a composite > field > could add the subfields to the model class if they aren't defined > explicitly > and their names passed in arguments to the composite field. > > 3) Have you checked that all the Field APIs make sense for CompositeField? > It's quite obvious that value_from/to_dict are needed. It's less obvious > that > everything else is still needed. > > 4) I'm wary of the extra 'isnull' column. Couldn't we required that, if > the > composite field is NULL, at least one of the subfields is NULL? My idea is > that a nullable composite field would consider itself NULL if all nullable > subfields are NULL. Declaring a composite subfield nullable when all > subfields > are non-nullable would be an error. In your MoneyField example, the amount > subfield should be nullable when the composite field is nullable. > > 5) I understand the first two restrictions. They required deeper > refactorings > to be lifted. The reason for the third one is less clear. Is it just a > matter > of beating the metaclass into cooperation? > > Best, > > -- > Aymeric. > -- 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/e1024af3-2822-4136-979c-7980e8c99dc5%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
