#6095: Add the ability to manually create M2M intermediary models
------------------------+---------------------------------------------------
Reporter: jacob | Owner: floguy
Status: assigned | Component: Database wrapper
Version: SVN | Resolution:
Keywords: | Stage: Accepted
Has_patch: 1 | Needs_docs: 1
Needs_tests: 0 | Needs_better_patch: 1
------------------------+---------------------------------------------------
Comment (by russellm):
Replying to [comment:42 floguy]:
> Can someone please try out this new patch and let me know if it works
for them? I'm using git now to make development on this ticket easier,
and this patch applies cleanly for me but I'm not sure if it'll apply to
anyone else.
The !__init!__.py didn't roll out on my end, but it otherwise applies
cleanly.
The patch is starting to look pretty good, though. A few more (mostly
minor) comments:
1. I'm not sure that NotImplementedError is the right approach for the
add/remove methods; I'd be inclined to just remove them (or not define
them in the first place). NotImplemented implies that we'll get around to
it one day - but we wont, because they can't ever work. If the method is
completely missing, it won't turn up in autocompletes or otherwise lead to
the temptation or suggestion that it will work.
2. The GenericRel special case in sql.py is a bit of a wart; I can see
why you've appended a new condition to this check, but I'm wondering if we
can't abstract this out - i.e., put an attribute on a many_to_many field
that would disable database table creation?
3. A couple of typos in error messages - non-exhaustive list: "manualy",
"Cannot set values on a ManyToManyFields which specify a through model"
4. get_reverse_rel_field() - firstly, related_name is an unused argument;
secondly, you can make this a little simpler (and more robust) using
_meta.get_all_related_objects() (and possibly suggests an improvement
required in the meta class - get_related_object(name) and
get_related_many_to_many_object(name)).
5. Minor doctest note: the modeltests serve a dual purpose, both as a
regression test, and a source of examples. As such, it's helpful if the
doctest comments explain what the doctest is actually testing. The
comments that are currently there are are all correct, but not
particularly illuminating for a newcomer. For example, the query
docstrings would be more helpful if they were of the form "Find all the
groups that have a member named 'Bob'; "Queries also work in the reverse
direction: find all the People in the group called "Rock".
6. Admin integration. If you put the test case models into a project, and
try to add a Group, you get m2m widgets for members, custom_members and
nodefaultsnonulls, which then fail on save because add() is not
implemented. This may be something that needs to be addressed in newforms-
admin, rather than patching the existing admin app.
7. Regarding the documentation; I'm not sure the PizzaTopping example
really illuminates the problem (and solution) that clearly. IMHO, the
Person/Group/Membership is a really natural example - why not stick with
it?
8. Again on the documentation: Most users are coming to the documentation
with a problem that they want to have solved (How do I associate data with
an m2m relation?). However, the 'flavour' of the docs is very much one of
'this is how intermediate tables are created and used'. While the
implementation of the solution is important, and may well be illuminating,
it distracts from the explanation of the problem you are solving.
That should give you plenty to work on :-)
On a side note - thanks for the nice words on This Week in Django :-).
--
Ticket URL: <http://code.djangoproject.com/ticket/6095#comment:45>
Django Code <http://code.djangoproject.com/>
The web framework for perfectionists with deadlines
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en
-~----------~----~----~----~------~----~------~--~---
