Opened 19 years ago

Closed 19 years ago

Last modified 18 years ago

#122 closed enhancement (fixed)

[patch] Build models using fieldname=FieldClass

Reported by: anonymous Owned by: Adrian Holovaty
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: mmarshall@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is a patch to make fieldname=FieldClass type model definitions possible. It is fully backwards-compatible with the fields=(...) method.

The example given in the tutorial could be re-written as such:

from django.core import meta

class Poll(meta.Model):
    question = meta.CharField(maxlength=200)
    pub_date = meta.DateTimeField('date published')
    
class Choice(meta.Model):
    ForeignKey = Poll
    choice = meta.CharField(maxlength=200)
    votes = meta.IntegerField()
    

Other ways of defining a ForeignKey:

#multiple keys can be used with tuples:
ForeignKey = Poll, OtherKey

#options can also be used:
ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3}

#the attribute name is irrelevant here:
anything = ForeignKey(Poll, edit_inline=True, num_in_admin=3)

The implementation is quite simple. (or horribly hackish, depending on how you see things.) It simply goes through all of the attributes of the class, and adds all of the instances of 'Field' to the fields list. (after updating their names with the attribute name.) It also processes the ForeignKey field, producing ForeignKeys as needed.

Provisions are in place to maintain the order of the Fields.

Here is the patch:

 Index: django/core/meta.py
===================================================================
--- django/core/meta.py (revision 253)
+++ django/core/meta.py (working copy)
@@ -368,6 +368,40 @@
         if not bases:
             return type.__new__(cls, name, bases, attrs)

+        # We must refactor the attributes around a little.  All Field class instances will be given
+        # names (as needed) and moved to the fields list.
+
+        attrs["fields"] = attrs.has_key("fields") and list(attrs["fields"]) or []
+
+        def handle_ForeignKey(obj):
+            if isinstance(obj, Model):
+                attrs["fields"].append(ForeignKey(obj))
+            elif type(obj) in (list, tuple):
+                if isinstance(obj[0], ModelBase) and type(obj[1]) == dict:
+                    attrs["fields"].append(ForeignKey(obj[0], **obj[1]))
+                else:
+                    for i in obj: handle_ForeignKey(i)
+            else:
+                raise ValueError("Cannot make ForeignKey from "+str(obj))
+
+        def handle_field(obj, name):
+            if isinstance(obj, Field):  #Check if this is a field
+                if not isinstance(obj,ForeignKey): obj.set_name(name)
+                attrs["fields"].append(obj)
+                return True
+            elif name == "ForeignKey":
+                handle_ForeignKey(obj)
+                return True
+            return False
+
+        # loop through the attributes, and take out the fields.
+        for key in attrs.keys()[:]:
+            if handle_field(attrs[key], key):
+                del attrs[key]  # If the attr was added to fields, we want to delete it.
+
+        # Sort the fields, so that they appear in the correct order.
+        attrs["fields"].sort(lambda x,y: x.creation_counter - y.creation_counter)
+
         # If this model is a subclass of another Model, create an Options
         # object by first copying the base class's _meta and then updating it
         # with the overrides from this class.
@@ -1551,14 +1585,19 @@
     # database level.
     empty_strings_allowed = True

-    def __init__(self, name, verbose_name=None, primary_key=False,
+    # Will be increaced each time a Field object is instanced.  Used to
+    # retain the order of fields.
+    creation_counter = 0
+
+    def __init__(self, name=None, verbose_name=None, primary_key=False,
         maxlength=None, unique=False, blank=False, null=False, db_index=None,
         core=False, rel=None, default=NOT_PROVIDED, editable=True,
         prepopulate_from=None, unique_for_date=None, unique_for_month=None,
         unique_for_year=None, validator_list=None, choices=None, radio_admin=None,
         help_text=''):
         self.name = name
-        self.verbose_name = verbose_name or name.replace('_', ' ')
+        self.original_verbose_name = verbose_name
+        self.verbose_name = verbose_name or name and name.replace('_', ' ')
         self.primary_key = primary_key
         self.maxlength, self.unique = maxlength, unique
         self.blank, self.null = blank, null
@@ -1583,6 +1622,24 @@
         else:
             self.db_index = db_index

+        # Increace the creation counter, and save our local copy.
+        self.creation_counter = Field.creation_counter
+        Field.creation_counter += 1
+
+    def set_name(self, name ):
+        """
+        Sets the name, and generates the verbose_name from it if needed.
+        This function is here for when the name needs to be set later, 
+        (such as if it needs to be obtained from the attribute name that
+        stores the Field instance.)
+        """
+        if not self.original_verbose_name:
+            # If the verbose name was originally not specified, we will assume that
+            # the user intends for the first argument passed to __init__ to be the verbose_name.
+            self.verbose_name = self.name
+
+        self.name = name
+        self.verbose_name = self.verbose_name or name and name.replace('_', ' ')
+
     def pre_save(self, obj, value, add):
         """
         Hook for altering the object obj based on the value of this field and

Attachments (9)

meta3.diff (4.7 KB ) - added by mmarshall 19 years ago.
A third version of the patch. Better support for ForeignKey, ManyToManyField, and OneToOneField.
meta4.diff (6.4 KB ) - added by mmarshall 19 years ago.
Patch version 4... Cuts out a bunch of the options.
proof_of_concept.py (1.4 KB ) - added by rmunn@… 19 years ago.
Proof-of-concept for ForeignKeys without assignment
meta5.diff (8.1 KB ) - added by mmarshall 19 years ago.
Added support for relationship fields not needing to be assigned to an attribute.
meta6.diff (8.2 KB ) - added by mmarshall 19 years ago.
Fixed a bug with relationship fields in the 'fields' attribute being duplicated. (oops)
model_api.diff (38.6 KB ) - added by mmarshall 19 years ago.
Implements the new model api. Breaks compatibility with old models.
model_api.r.389.diff (38.0 KB ) - added by mmarshall 19 years ago.
Same as before, only resolves confilicts with r.389
modeldiscuss.txt (31.9 KB ) - added by hugo <gb@…> 19 years ago.
IRC discussion about the model syntax
modeldiscuss_cleanedup.txt (34.0 KB ) - added by rmunn@… 19 years ago.
IRC discussion with tabs cleaned up so it's easier to read

Download all attachments as: .zip

Change History (53)

comment:1 by mmarshall, 19 years ago

Allright, I have an updated version here.

Now, you can optionally place your fields under "class Fields:" and your meta data under "class Meta:". Note that you can still put everything directly in your Model class, or you can put some {Fields|Meta} info under "class {Fields|Meta}:" and put some directly under your Model class.

Here are some examples of what you can do: (Previously given example still works.)

from django.core import meta

class Poll(meta.Model):
    question = meta.CharField(maxlength=200)
    pub_date = meta.DateTimeField('date published')
    
    class Meta:
        module_name = "polls"
        admin = meta.Admin(list_display = ('question', 'pub_date'))

class Choice(meta.Model):
    class Field:
        ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3}
        choice = meta.CharField(maxlength=200, core=True)
        votes = meta.IntegerField(core=True)
class SomeModel(meta.Model):
    afield = meta.IntegerField()
    class Field:
        admin = meta.CharField()

    module_name="some_models"
    admin = meta.Admin()

Here is the patch.

Index: django/core/meta.py
===================================================================
--- django/core/meta.py (revision 256)
+++ django/core/meta.py (working copy)
@@ -368,6 +368,56 @@
         if not bases:
             return type.__new__(cls, name, bases, attrs)

+        # We must refactor the attributes around a little.  All Field class instances will be given
+        # names (as needed) and moved to the fields list.
+
+        attrs["fields"] = attrs.has_key("fields") and list(attrs["fields"]) or []
+
+        def handle_ForeignKey(obj):
+            if isinstance(obj, Model):
+                attrs["fields"].append(ForeignKey(obj))
+            elif type(obj) in (list, tuple):
+                if isinstance(obj[0], ModelBase) and type(obj[1]) == dict:
+                    attrs["fields"].append(ForeignKey(obj[0], **obj[1]))
+                else:
+                    for i in obj: handle_ForeignKey(i)
+            else:
+                raise ValueError("Cannot make ForeignKey from "+str(obj))
+
+        def handle_field(obj, name):
+            if isinstance(obj, Field):  #Check if this is a field
+                if not isinstance(obj,ForeignKey): obj.set_name(name)
+                attrs["fields"].append(obj)
+                return True
+            elif name == "ForeignKey":
+                handle_ForeignKey(obj)
+                return True
+            return False
+
+        # loop through the attributes, and take out the fields.
+        for key in attrs.keys()[:]:
+            if handle_field(attrs[key], key):
+                del attrs[key]  # If the attr was added to fields, we want to delete it.
+
+        try:
+            for name in dir(attrs["Field"]):
+                handle_field(getattr(attrs["Field"],name),name)
+            del attrs["Field"]
+        except KeyError: pass
+
+        try:
+            for name in dir(attrs["Meta"]):
+                if name.startswith("__"): continue
+                if name in ("fields",):
+                    attrs[name] += getattr(attrs["Meta"],name)
+                else:
+                    attrs[name] = getattr(attrs["Meta"],name)
+            del attrs["Meta"]
+        except KeyError: pass
+
+        # Sort the fields, so that they appear in the correct order.
+        attrs["fields"].sort(lambda x,y: x.creation_counter - y.creation_counter)
+
         # If this model is a subclass of another Model, create an Options
         # object by first copying the base class's _meta and then updating it
         # with the overrides from this class.
@@ -1551,14 +1601,19 @@
     # database level.
     empty_strings_allowed = True

-    def __init__(self, name, verbose_name=None, primary_key=False,
+    # Will be increaced each time a Field object is instanced.  Used to
+    # retain the order of fields.
+    creation_counter = 0
+
+    def __init__(self, name=None, verbose_name=None, primary_key=False,
         maxlength=None, unique=False, blank=False, null=False, db_index=None,
         core=False, rel=None, default=NOT_PROVIDED, editable=True,
         prepopulate_from=None, unique_for_date=None, unique_for_month=None,
         unique_for_year=None, validator_list=None, choices=None, radio_admin=None,
         help_text=''):
         self.name = name
-        self.verbose_name = verbose_name or name.replace('_', ' ')
+        self.original_verbose_name = verbose_name
+        self.verbose_name = verbose_name or name and name.replace('_', ' ')
         self.primary_key = primary_key
         self.maxlength, self.unique = maxlength, unique
         self.blank, self.null = blank, null
@@ -1583,6 +1638,24 @@
         else:
             self.db_index = db_index

+        # Increace the creation counter, and save our local copy.
+        self.creation_counter = Field.creation_counter
+        Field.creation_counter += 1
+
+    def set_name(self, name ):
+        """
+        Sets the name, and generates the verbose_name from it if needed.
+        This function is here for when the name needs to be set later, (such as if it needs to be obtained from
+        a the attribute name that stores the Field instance.)
+        """
+        if not self.original_verbose_name:
+            # If the verbose name was originally not specified, we will assume that
+            # the user intends for the first argument passed to __init__ to be the verbose_name.
+            self.verbose_name = self.name
+
+        self.name = name
+        self.verbose_name = self.verbose_name or name and name.replace('_', ' ')
+
     def pre_save(self, obj, value, add):
         """
         Hook for altering the object obj based on the value of this field and

comment:2 by mmarshall, 19 years ago

Oh, there is something I forgot to mention. Notice how in the examples I included "module_name" in the examples? It seems that the automatic name generation gets messed up if you put a class inside of your model class. (I think this also happens if you use method names with upper case.)

I also just realized that I didn't do anything for ManyToManyField. Although you can simply do fields=(meta.ManyToManyField(...),), it probably should be implemented the same as ForeignKey.

comment:3 by stephen@…, 19 years ago

The following case should be taken into consideration. This code could not work because there is a mutual relationship where both tables have a FK, relating to each other.

sqlobject does this by passing table names as a string, not an object.

from django.core import meta

class Person(meta.Model):
    name = meta.CharField(maxlength=200)
    place_of_residence = ForeignKey(Address) # Error Occurs Here
    
class Address(meta.Model):
    owner = ForeignKey(Person)
    suburb = meta.CharField(maxlength=200)
    postcode = meta.IntegerField()

comment:4 by mmarshall, 19 years ago

hmm... Wouldn't it be better to do this?

from django.core import meta

class Person(meta.Model):
    name = meta.CharField(maxlength=200)
    
class Address(meta.Model):
    owner = meta.OneToOneField(Person)
    suburb = meta.CharField(maxlength=200)
    postcode = meta.IntegerField()

BTW, looking at this line:

owner = ForeignKey(Person)

I guess it would be best for the attribute name (owner in this case) to be used as the rel_name for ForeignKey? (Right now in this patch it is simply discarded)

by mmarshall, 19 years ago

Attachment: meta3.diff added

A third version of the patch. Better support for ForeignKey, ManyToManyField, and OneToOneField.

comment:5 by mmarshall, 19 years ago

Ok, I just attached a new version of the patch.

Something like this:

    owner = ForeignKey(Person)

... now works the same as this:

    fields = (
        ForeignKey(Person, rel_name="owner"),
    )

ManyToManyField and OneToOneField should work in this way as well. (I haven't tested...)

comment:6 by stephen@…, 19 years ago

mmarshall:
nope, A person can own many properties, and might live in a property he doesn't own.

Also, throwing away the name of the relationship causes problems, as the following kind of situation may occur:

from django.core import meta

class Person(meta.Model):
    name = meta.CharField(maxlength=200)
    
class CourtCase(meta.Model):
    plaintiff = meta.ForeignKey(Person)
    defendant = meta.ForeignKey(Person)
  • the name of the relationship is very important.
  • it should be possible to have foreignkeys refer to tables via their name, not by passing the object, to avoid the chicken/egg problem detailed in my previous code example.

A tried and tested python ORM API is sqlobject, which handles both these cases, maybe we should take some lessons from there:

SQLobject api docs

comment:7 by stephen@…, 19 years ago

Oh, just reread comments. The one previous to my last one talks about a patch that doesn't throw away the relationship name. I didn't realise that.

Sorry. The only remaining issue I see is the mutual FK relationship. We need to be able to have a FK in a class that doesn't yet exist.

This will also cause problems when automatically generating the tables in the database. You have to define the tables first, then add the FKs, to avoid database constraints from blowing up.

comment:8 by mmarshall, 19 years ago

What about ManyToManyField? Doesn't that handle what you want?

comment:9 by Adrian Holovaty, 19 years ago

I like the fieldname = meta.Field(...) syntax in general, but here are a few issues I have with this particular implementation:

  • It's too flexible, to the point of confusion. Making it possible to do either class Meta or class Field or plain class attributes just smacks of "there's more than one way to do it." There should be one, clear, obvious way to do it. If we decide to change model syntax, let's have class Meta for non-field info, and all fields are just attributes of the class.
  • The current ForeignKey solution rubs me the wrong way. ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3} just doesn't feel natural -- or look good. I still haven't seen a good way to designate ForeignKeys in class attribute syntax without reverting to cruft.

I'd love to see ideas/patches that use the nicer fieldname = meta.Field(...) syntax but avoid these two issues.

comment:10 by rmunn@…, 19 years ago

I like adrian's suggestion of having just class Meta for non-field info and all other fields are just written as attributes of the class. +1.

One concern, in the remote-possibility range: what if someone absolutely must have a field named "Meta", with that initial capital, and can't change it? Is there a way to do this even with a class.Meta in the equation? Perhaps (strawman syntax): "othername = meta.Field(override_name="Meta")?"

This probably isn't too terribly important, though, and I suspect if it really becomes necessary it would be possible to shoehorn it in.

comment:11 by mmarshall, 19 years ago

Thanks for the feedback!

"""
either class Meta or class Field or plain class attributes just smacks of "there's more than one way to do it."
"""

I agree. I implemented all three ways because there were people who like each one. I didn't know which one I liked best, and it took very little more code to implement them all. If you want just the 'class Meta:' method, I can implement that just by deleting a little code.

"""
The current ForeignKey solution rubs me the wrong way. ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3} just doesn't feel natural -- or look good.
"""

I agree. So, perhaps it should be limited to "ForeignKey = Poll" for when no options are needed? and use "rel_name = meta.ForeignKey(RelatedObject)" for when more options are needed? Or just scrap the former?

MWM

comment:12 by mmarshall, 19 years ago

rmunn:

If someone absolutely _must_ have a field named "Meta", they can use "field=(meta.ForeignKey(RelatedObject, rel_name='Meta'),)" and use the new way for all the other fields.

MWM

comment:13 by rmunn@…, 19 years ago

I say stay consistent: treat ForeignKeys like any other field type, and make it "rel_name = meta.ForeignKey(RelatedObject)" even if no other options are needed. Switching between "ForeignKey = Poll" and "poll = ForeignKey(Poll, null=True, blank=True)" would be very jarring otherwise.

I believe there's been a suggestion on the django-developers list to allow something like "poll = ForeignKey('Poll')" and do a name-based lookup later, to allow for circular references of ForeignKeys, but maybe that should be a separate patch.

comment:14 by Adrian Holovaty, 19 years ago

Regarding ForeignKeys --

The small problem with "rel_name = meta.ForeignKey(RelatedObject)" is that users shouldn't have to type the rel_name unless they specifically want to have control over it. That's the sort of thing that should be automated unless absolutely necessary.

Any way to use "meta.ForeignKey(RelatedObject)" without having to specify the rel_name?

comment:15 by rmunn@…, 19 years ago

Suggestion for the latest version of the patch: the line "attrs += getattr(attrsMeta, name)" (after "if name in ("fields",):") should probably be "attrs.extend(getattr(attrsMeta, name))". I don't like extending lists with the += operator, since its operation on (mutable) lists is different from its operation on other, immutable values like strings or ints or even tuples. That's too "magical" for my taste; list.extend is better because it's explicit about what's going on.

comment:16 by rmunn@…, 19 years ago

To use "meta.ForeignKey(RelatedObject)" without having to specify the rel_name will probably have to involve some trickery with stack frames. I think it would be possible, though: I'm playing with some ideas. I'll post my results once I have 'em.

by mmarshall, 19 years ago

Attachment: meta4.diff added

Patch version 4... Cuts out a bunch of the options.

comment:17 by mmarshall, 19 years ago

Allright, I have a new version of the patch attached. I cut out a bunch of the options. 'ForeignKey = RelatedObject' is no longer implemented, and the 'class Fields:' is no longer there. You can, however, still specify meta data in either 'class Meta:' or as top level attributes. (This is for backwards compatibility.)

@Adrian
Yes, this is why I came up with the 'ForeignKey = RelatedObject' form. And yes, I think that "meta.ForeignKey(RelatedObject)" without assigning it to anything is possible, but it would require a bit of magic. Should I work on that? (And, I assume, do the same for ManyToManyField and OneToOneField?)

@rmunn
I think that the way I had it looks cleaner, but whatever. It's changed :)

MWM

comment:18 by Adrian Holovaty, 19 years ago

mmarshall -- Sure, if you can get meta.ForeignKey(RelatedObject) to work without assigning it to anything, and it works, and it doesn't have too much overhead, that'll be the final step before we integrate this into Django, as far as I'm concerned. I'll need to talk about it with Jacob, but I believe he thinks it's an improvement.

For backwards compatibility, what do you think of these proposals:

  • We could call the new syntax meta.NewModel instead of meta.Model. That way meta.Model would still work, until the first official release, when we'd rename meta.Model to meta.OldModel and meta.NewModel to meta.Model. Eventually we'd remove meta.OldModel.
  • Or, we could immediately rename meta.Model to meta.OldModel and use the new meta.Model immediately. For people who don't have time to convert their models to the new syntax, they could use meta.OldModel for a quick/easy way to fix their code in the interim. Then meta.OldModel would be removed, probably at first release.

Other than that, don't worry about backwards compatibility within your code -- make the code as fast and clean as possible.

comment:19 by rmunn@…, 19 years ago

I'm experimenting with ways to do meta.ForeignKey(RelatedObject) without assigning it; I'll post a proof-of-concept when I'm done. (Soon).

comment:20 by mmarshall, 19 years ago

About backwords compatibility... I have been making it backwards compatible because that is the easy way out. In case if you haven't looked carefully at the patch, all that I am doing is:

  • Taking all of the attributes that inherent from Field, assigning the attribute name to them, and adding them to the fields list.
  • A little hack to make the first arg passed to a field be the verbose_name when applicable.
  • Copying the attributes from the Meta class, and placing them in the main class attributes.

So, the only time my code comes into play is when python parses the class. From then on, it's all just the same as it was before.

I chose this route because I knew that the old method with the 'fields' attribute has been well tested. If I revamp all of that, I am afraid I would introduce all sorts of bugs that won't pop up for a while.

However, I suppose that this also is another layer of complexity. Having the Model classes go through this conversion could be confusing for someone else trying to get into the code.

So, Adrian, what do you think? Would it be worth it to delve into refactoring meta.py? Or should I just give some clear documentation of what is going on with the conversion? (I am inclined to the latter.)

by rmunn@…, 19 years ago

Attachment: proof_of_concept.py added

Proof-of-concept for ForeignKeys without assignment

comment:21 by rmunn@…, 19 years ago

Okay, I've got proof-of-concept code up. Look at the definition of SampleRecord, then run it and notice that the ForeignKeys know what their name is, and that SampleRecord got assigned the right attributes.

by mmarshall, 19 years ago

Attachment: meta5.diff added

Added support for relationship fields not needing to be assigned to an attribute.

comment:22 by mmarshall, 19 years ago

I just attached a new version of the patch that supports relationship fields not being assigned to an attribute. Great idea rmunn!

This works with ForeignKey, ManyToManyField, and OneToOneField.

MWM

by mmarshall, 19 years ago

Attachment: meta6.diff added

Fixed a bug with relationship fields in the 'fields' attribute being duplicated. (oops)

comment:23 by mmarshall, 19 years ago

Hey Adrian,

I was just thinking more about my comment concerning backwards compatibility, and trying to avoid refactoring a large amount of the code. There might be some good reasons for refactoring.

First of all, I would like to see everything that has to do with a field inside of that field's class. I don't think that stuff like ""isinstance(f, FileField):"" belongs in Model.new. (Relationship fields might be a good exception for this, however.) It might also be good to put the Field classes in their own module, cutting a fair bit off of the 2200+ line beast of meta.py ;-) This should also make it easier for a user to create a custom Field subclass, without having to modify django itself.

Also related to Fields, I am currently doing some dirty hacks to get name/verbose_name sorted out. With this new method, verbose_name should be made the first argument, in order to clean stuff up.

Ok, I have convinced myself at least that this is the better way to go.

Thoughts?

MWM

comment:24 by mmarshall, 19 years ago

Oh, and when I said "There might be some good reasons for refactoring.", I meant more along the lines of, "There might be some other reasons for refactoring that I hadn't thought of before."

MWM

comment:25 by Adrian Holovaty, 19 years ago

I closed #91, which was a duplicate of this.

comment:26 by Adrian Holovaty, 19 years ago

milestone: Version 1.0

by mmarshall, 19 years ago

Attachment: model_api.diff added

Implements the new model api. Breaks compatibility with old models.

by mmarshall, 19 years ago

Attachment: model_api.r.389.diff added

Same as before, only resolves confilicts with r.389

comment:27 by mmarshall, 19 years ago

Allright, so these latest patches break compatibility. Here are some notes, in the order that they come to mind:

First two args for fields are now reversed. (verbose_name before name)

add_fields is not supported. Fields are inhereted like normal attributes. (Come to think of it, there wasn't a test for this, so it might not work.)

All fields can be added without assigning to an attribute. (Opposed to just the relationship fields in previous patches.) This was done because it was the easiest and cleanest way to do it, not to have a gazzillion ways of doing something. ;)

I had wanted to get rid of many of those 'isinstance(f, SomeFieldClass)'s, but I didn't. This is enough code change for one patch :)

I ported all of the test models. They all pass for me.

It is thread-safe. (Or at least it should be.)

Thoughts?

MWM

comment:28 by Adrian Holovaty, 19 years ago

Summary: Build models using fieldname=FieldClass[patch] Build models using fieldname=FieldClass

comment:29 by garthk@…, 19 years ago

As Syndrome might put it, I'm completely geeking out over the technical wizardry behind not having to actually assign an attribute to get a field. I'm just as worried, though, that there's perhaps a litle *too* much magic in there. I wouldn't be at all surprised if the five seconds saved by not having to give an attribute name would be wiped out several times over in confusion amongst code maintainers that, whilst Python-aware, might not necessarily know Django.

It's not just a matter of readability-to-humans, either. I can't imagine all IDEs, code checkers, documentation generators, and API browsers will cope. If they can't, then you're making a whole bunch of people's lives a whole lot more difficult just to save others a few keystrokes.

What's the problem with making the model look like Python classes that makes it preferable to implement some weird bucket of expressions that build a class in a way unlike any other class a Python programmer has ever encountered?

comment:30 by garthk@…, 19 years ago

Another hazard is that stackframe wizard is vulnerable to changes in the stackframes. You'll need to widen your test footprint to -O, Psyco, PyPy, IronPython, Jython, every new version of Python, running under wxPython, and goodness knows what else if you rely on it. I had massive problems with my lock debugging code in PyDS just from -O alone.

comment:31 by mmarshall, 19 years ago

Cc: mmarshall@… added

With the way that django pieces together classes using metaclassing, (and modules for that matter,) I don't think that this patch will break IDEs and code checkers any more than they are already broken. :-D

As for messing with the stack, I hadn't thought about there being difficulties with other python implementations. I don't imagine anyone trying doing it on PyPy, IronPython, and Jython in the near future, but -O, and maybe Psyco would be worth testing with. Even then, IMHO Psyco takes up more memory than it's worth in a webapp, and AFAIK -O is considered unstable anyway.

Have you taken a close look at my patch? Inspecting the stack is mainly just there for multithreading. (Although there will be a couple of trivial issues that pop up if I remove it.)

MWM

comment:32 by anonymous, 19 years ago

I just tested it with -O and Psyco. All tests pass fine with -O, but Psyco has problems with the inspect module. However, with Psyco you cannot dynamically modify member functions, so I don't think it would work anyway.

MWM

comment:33 by Adrian Holovaty, 19 years ago

Resolution: wontfix
Status: newclosed

Jacob and I have talked this over at length, and we've decided the model syntax shouldn't change. Using a fieldname=FieldClass syntax would require too much "magic" behind the scenes for minimal benefit.

comment:34 by garthk@…, 19 years ago

That's a pity. fieldname=meta.Whatever(...) feels right. :|

comment:35 by Adrian Holovaty, 19 years ago

Resolution: wontfix
Status: closedreopened

Reopening after IRC discussion.

by hugo <gb@…>, 19 years ago

Attachment: modeldiscuss.txt added

IRC discussion about the model syntax

by rmunn@…, 19 years ago

Attachment: modeldiscuss_cleanedup.txt added

IRC discussion with tabs cleaned up so it's easier to read

comment:36 by rmunn@…, 19 years ago

The tabs in the previous file were making text from me (rmunn) and hugo- on a different indentation level than everyone else's text, so I cleaned up the file to use spaces instead. Now it should be a lot easier to read. :-)

comment:37 by garthk@…, 19 years ago

Assuming I'm reading the transcript correctly: are we back to something like the following?

from django.core import meta

class Person(meta.Model):
    name = meta.CharField(maxlength=200)
    
class CourtCase(meta.Model):
    plaintiff = meta.ForeignKey(Person)
    defendant = meta.ForeignKey(Person)

I'm definitely +1 on that.

Do we need a separate ticket for direct assignment and reference of objects in related tables, or can we squeeze it in on the same change as this ticket?

me = persons.get_object(name__exact="garthk")
you = persons.get_object(name__exact="hugo-")
case = courtcases.CourtCase()
case.defendant = me
case.plaintiff = you
case.save()
print case.defendant.name

You'd also want to retain support for case.defendant_id = me.id of course, but I keep stumbling over not being able to do it the obvious way.

comment:38 by Manuzhai, 19 years ago

garthk: I'd say we would use a different ticket for that, I concur with your views (although I haven't checked the code to see how hard an implementation would be).

comment:39 by garthk@…, 19 years ago

Just had a chat to hugo, and I'd also like to +1 his suggestion that there also be some model creation API through which he can generate his thousand-field tables programmatically. It makes sense that the new model syntax invoke that API.

Whilst I'm posting, I suspect the current convention would be that the way to let Django know about the administration model would be:

class Poll(meta.Model):
    __admin__ = meta.Admin()
    question = meta.CharField(maxlength=30)
    pub_date = meta.DateTimeField()
    sites = meta.ManyToManyField(core.Site)

rather than:

class Poll(meta.Model):
    question = meta.CharField(maxlength=30)
    pub_date = meta.DateTimeField()
    sites = meta.ManyToManyField(core.Site)
    class Meta:
        admin = meta.Admin()

comment:40 by Manuzhai, 19 years ago

I actually, I think the convention for metadata would still be a class Meta, although I suggested this:

class Poll(meta.Model):
    question = meta.CharField(maxlength = 30)
    pub_date = meta.DateTimeField()
    sites = meta.ManyToManyField(core.Site)
    class META:
        admin = meta.Admin()

I'm not sure what the contents for the meta-class would look like, though.

comment:41 by garthk@…, 19 years ago

Following Manuzhai's suggestion, I've moved my related field access idea to #359.

comment:42 by Adrian Holovaty, 19 years ago

Resolution: fixed
Status: reopenedclosed

(In [549]) Fixed #122 -- BIG, BACKWARDS-INCOMPATIBLE CHANGE. Changed model syntax to use fieldname=FieldClass() syntax. See ModelSyntaxChangeInstructions for important information on how to change your models

comment:43 by Seer, 18 years ago

Hi all
im fine, gl all!

comment:44 by (none), 18 years ago

milestone: Version 1.0

Milestone Version 1.0 deleted

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