Fixed #9475 -- Allowed add(), create(), etc for m2m with through #9609
Conversation
… model has required fields. Added test to add,create, and set for m2m without through_defaults kwarg
…sing a list of dictionaries to through_defaults.
| >>> beatles.members.set([john, paul, ringo, george], through_defaults={'date_joined': date(1960, 8, 1)}) | ||
|
|
||
| For ``add()`` and ``set()`` you may either specify a single 'default' dictionary or a list of dictionaries | ||
| if you wish to specify details for each object. |
There was a problem hiding this comment.
Hmm... If we do this, maybe the kwarg should just be called intermediate_values everywhere (that can be either a single dict or list of dicts)
I feel like passing in two separate lists and relying matching indexes is a little messy, and looking at the code, I've already found some possible bugs.
There was a problem hiding this comment.
Though, I suppose the "through_defaults" does make a lot of sense for pretty much everywhere except add() and create(). So nevermind. I think _add_items should keep using through_defaults as the kwarg to be consistant. You could use intermediate_values as a local variable if you want.
There was a problem hiding this comment.
I agree that passing two lists is messy as can be. My original intention was to only allow the dictionary of defaults, but then I realized that being able to set different values for different objects could be valuable. Mucked it up pretty hard after that.
It's hard to find clean ways to add values without breaking the interface. Clearly needs more thought :/
Good point about intermediate_values. I'm really not sure why I used a different name. Could have just been I forgot to update it to through_defaults.
@collinanderson Thanks for the feedback!
| if isinstance(intermediate_values, dict): | ||
| intermediate_values = [ | ||
| intermediate_values for obj in objs | ||
| ] |
There was a problem hiding this comment.
We should check to be sure len(intermediate_values) == len(objs).
There was a problem hiding this comment.
If we did length checking we'd want to compare len(intermediate_values) == len(new_ids) since these are the list getting zipped, and that won't really help because (I missed this originally) new_ids being a set which has all existing members removed makes the zip untenable.
| }) | ||
| for obj_id in new_ids | ||
| })) | ||
| for obj_id, extra_values in zip(new_ids, intermediate_values) |
There was a problem hiding this comment.
keep in mind new_ids could be smaller than objs, messing up index order so intermediate_values won't match up, right? Also, new_ids is a set, so order is lost.
There was a problem hiding this comment.
Yep, missed both those points originally. Clearly needs a rework....
carltongibson
changed the title
|
Dear astandley & Collin Anderson, We are working on identifying redundant development and duplicated pull requests. We have found there is a closed pull request: #8222 which might be similar to this one. We would really appreciate if you could help us to validate and give us some feedback. Thank you very much for your time! Sincerely, |
|
@FancyCoder0 Yes, this PR replaces #8222. |
|
#8981 is now the active version of this. |
Updated Fix for https://code.djangoproject.com/ticket/9475 -- allow add(), create(), etc for m2m with through