[Repoze-dev] repoze.folder API weirdness
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. Wichert. ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
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) Wichert. ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
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
Re: [Repoze-dev] repoze.folder API weirdness
Am 17.06.2010, 11:28 Uhr, schrieb Wichert Akkerman wich...@wiggy.net: 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. This is standard behaviour for folders (the not accepting duplicates). I think changing it would be against user expectations. Charlie -- Charlie Clark Managing Director Clark Consulting Research German Office Helmholtzstr. 20 Düsseldorf D- 40215 Tel: +49-211-600-3657 Mobile: +49-178-782-6226 ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
On 17 June 2010 16:08, Charlie Clark charlie.cl...@clark-consulting.eu wrote: This is standard behaviour for folders (the not accepting duplicates). I think changing it would be against user expectations. My file system accepts duplicates (meaning replacement). I don't need to remove the file first. \malthe ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
Am 17.06.2010, 16:08 Uhr, schrieb Tim Hoffman zutes...@gmail.com: I would be concerned if you tried to __setitem__ a new page to replace the existing folder of the same name thus blowing away a whole heap of subfolders and documents in the process. Agreed but mv will let you do this but there are things in the shell that shouldn't necessarily be emulated... Charlie -- Charlie Clark Managing Director Clark Consulting Research German Office Helmholtzstr. 20 Düsseldorf D- 40215 Tel: +49-211-600-3657 Mobile: +49-178-782-6226 ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
On Thu, Jun 17, 2010 at 10:08 AM, Tim Hoffman zutes...@gmail.com wrote: I would be concerned if you tried to __setitem__ a new page to replace the existing folder of the same name thus blowing away a whole heap of subfolders and documents in the process. Very bad ;-( I would tend to think that responsibility for protecting the user from errors lies at the UI and application level. An API, generally speaking, should just do what it's told. I don't think I have a preference between wiggy's and mcdonc's proposals. Chris ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
Re: [Repoze-dev] repoze.folder API weirdness
For what its worth I would follow the current zope object manager semantics. Rgds T On Thu, Jun 17, 2010 at 10:41 PM, Chris Rossi ch...@archimedeanco.comwrote: On Thu, Jun 17, 2010 at 10:08 AM, Tim Hoffman zutes...@gmail.com wrote: I would be concerned if you tried to __setitem__ a new page to replace the existing folder of the same name thus blowing away a whole heap of subfolders and documents in the process. Very bad ;-( I would tend to think that responsibility for protecting the user from errors lies at the UI and application level. An API, generally speaking, should just do what it's told. I don't think I have a preference between wiggy's and mcdonc's proposals. Chris ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev ___ Repoze-dev mailing list Repoze-dev@lists.repoze.org http://lists.repoze.org/listinfo/repoze-dev
