Issue #23 wontfix

Use (custom) queryset instead of model classmethods.

Anonymous created an issue

Why not do things the django way and move the classmethods on the models to the custom querysets (currently used for NS and MP for delete only)? I'm talking about the add_root method in particular (which could just be an override of create).

So instead of doing:

{{{SomeTreeModel.add_root()}}}

you would be doing:

{{{SomeTreeModel.objects.add_root()}}}

or even better:

{{{SomeTreeModel.objects.create()}}}

since adding an instance without tree info is the same as adding a root.

This should also make the add_root() or create() available to related managers:

{{{some_instance.sometreemodel_set.add_root()}}}

will create a root of SomeTreeModel with the related instance set to some_instance, just like normal relations.

Comments (3)

  1. Gustavo Picon repo owner

    This will probably be better explained in a long blog post, but basically, IMO, the way most people use the Django ORM API is wrong. Using Django managers for everything is WRONG. Stop doing that.

    Let's take one of your examples:

    SomeTreeModel.objects.add_root()
    

    That doesn't make sense. Why do you want .add_root() in the manager? What's the point of something like:

    SomeTreeModel.objects.filter(desc='abcd').add_root()
    

    See my point? That doesn't make sense at all. You want to add a root node of class SomeTreeModel. It doesn't make any sense having .add_root() in the manager because .add_root() doesn't care about other objects in the model at all.

    For this very reason, Django's Model.objects.create() is a big API mistake. It's API smell. A better API would be Model.create(), but it's too late for Django now.

    It's not late for treebeard so I'm trying to do the right thing with the API :)

    Also, the API design of treebeard may be complex when compared to other libraries, but that's because trees are complex data structures. And to me the correct handling of a tree had priority over "Django API tradition".

    Note that I'm open to suggestions to improve the API and even add some backwards incompatible changes if needed (see #16), but in this case of managers vs class methods, I'll have to mark this ticket as wontfix.

  2. Anonymous

    Ok, I see your point and I even tend to agree about the create() method on managers.

    On second thought, it may not be the objects.create() that needs to be changed, but the save() method. There is no way to call save() on a model to create a root node, or any other node.

    ModelForms, for instance, are the main method of creating new objects by users (admin for example) and they call save(), which fails since the node does not know it should actually be a root, or even a child of a given parent, or a sibling of another node, or anything, because there are separate creation methods for that.

    So you may want to give it a thought to improve compatibility with normal django operations. I was just taking treebeard for a spin (since the alternative, mptt, seems to be dead) and this was something I ran into very quickly.

  3. 7mp

    I'd like to comment on this even though the issue is already closed.

    I'd be willing to implement said operations as manager methods since they would make the whole show work that much easier.

    Then a bit of argumenting: true, it doesn't make sense to have

    SomeTreeModel.objects.filter(desc='abcd').add_root()
    

    but in that case we wouldn't be dealing a manager method but with a QuerySet method (dealing with queries). But having just

    SomeTreeModel.objects.add_root()
    

    would be correct since then it would be a manager method – the "Django way" of implementing table-level operations.

    If you would be using Model.create instead of Model.objects.create, how would you make the distinction between databases in a multi-db environment?

    That said, would the issue be open for a patch? :)

  4. Log in to comment