On Thu, 2010-06-17 at 14:57 +0200, Wichert Akkerman wrote: > On 6/17/10 14:53 , Chris McDonough wrote: > > On Thu, 2010-06-17 at 11:28 +0200, Wichert Akkerman wrote: > >> I noticed something odd in repoze.folder: __setitem__ does not allow you > >> to replace an existing item. This is a result from __setitem__ being an > >> alias for add(). Is that a deliberate design decision? If not I'ld like > >> to change it to allow replacing items. > > > > The folder API isn't meant to exactly mirror the dictionary API, so I > > don't consider this odd. Most CMS-ish UI operations that call for > > adding a new item also call for aborting if an item by that name already > > exists, so we default to that behavior. > > > > But it probably doesn't matter much. As long as the deletion sends (or > > doesn't) the proper ObjectRemoved, etc events, I'd be sort of +0 on > > being able to replace an existing item. I think this would imply > > changing .add rather than changing __setitem__. Please read the > > docstrings for .add and .remove before changing much; we need to retain > > the ability to add and remove an item with and without sending events. > > If we change things so doing an addition replaces, and if someone adds a > > new item that already exists, .add should call .remove with the > > send_events argument the same value as what was passed to .add. > > My suggestion would be to make __setitem__ always do remove and add with > events. If someone needs more control they can explicitly call add and > remove directly. Allowing add() to replace items feels counterintuitive > to me. To put this in code my proposal would be: > > def __setitem__(self, key, item): > """Set a child item, optionally replacing an existing item with > the same key. This will always fire object events. If you want to > block events use add() and remove() directly. > """ > if key in self: > self.remove(key) > self.add(key, item)
Right now, the documentation says something like "__setitem__ works like add, but doesn't give you the control over sending events". Or vice versa. So you can say to someone "change the code so that you use add instead of __setitem__ here with the send_events=False, and it won't send events". In fact, I just described it to a customer that way yesterday. If we change __setitem__ to do remove, but .add stays as is, then the two operations become less isomorphic and harder to remember and explain. Let's just keep it simple. The only reason .add exists is because __setitem__ can't accept the send_events argument. And while it may be a poor name for something that also removes, so be it. I'd rather that the folder either didn't do replacement than for __setitem__ to not be isomorphic with .add. - C _______________________________________________ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
