Re: [Zope-dev] new BTreeContainer implementation, please review

2008-06-14 Thread Christophe Combelles

Marius Gedminas a écrit :

On Fri, Jun 13, 2008 at 09:40:06PM +0200, Christophe Combelles wrote:

While fixing some bugs in zope.app.container,
I've also modified the implementation of the BTreeContainer,
by not inheriting from the SampleContainer, and directly accessing the 
btree. This had remained as a TODO in the btree.py file, so I did it, 
but...


The result is all previous persisted BTreeContainers (such as 
PrincipalFolder) are broken because the btree used to be stored in 
self._SampleContainer__data, while the new implementation stores it in 
self._BTreeContainer__data.


So I've added a property to offer a transparent backward compatibility:

def _get__data(self):
try:
return self._BTreeContainer__data
except:


Please do not use bare except clauses.  Replace this with

  except AttributeError:


return self._SampleContainer__data
def _set__data(self, value):
try:
self._BTreeContainer__data = value
except:


When could this ever fail?


self._SampleContainer__data = value
def _del_data(self):
try:
del self._BTreeContainer__data
except:
del self._SampleContainer__data


Do we need __del__ at all?


__data = property(_get__data, _set__data, _del_data)



Do you think it is safe?


Not if you want compatibility in both directions.  If you want databases
created with the new zope.app.container to be readable with the old
zope.app.container (consider, e.g. a developer using a sandbox and
switching between bleeding-edge development and old maintenance
branches), then you can't do this.


Is there any better solution for this?


Perhaps a __setstate__ method to rename the attribute?  See the
PersistentMapping class in persistent/mapping.py for an example.

Should I 
rather write an evolution script?


*loathes*

Or should I revert all this back to 
inheriting from SampleContainer?


Or you could do like Benji York suggested and always use the
backwards-compatible name _SampleContainer__data.


thanks for the suggestions! I will do what Benji told, this seems the safest 
solution, and will allow to backport it to the 3.5 branch, so that the btree 
length calculation could also be fixed in zope 3.4.


Christophe



(Why oh why did SampleContainer want to use a double underscored name?)

Marius Gedminas




___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce

 http://mail.zope.org/mailman/listinfo/zope )


___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
http://mail.zope.org/mailman/listinfo/zope-announce

http://mail.zope.org/mailman/listinfo/zope )


Re: [Zope-dev] new BTreeContainer implementation, please review

2008-06-13 Thread Benji York
On Fri, Jun 13, 2008 at 3:40 PM, Christophe Combelles [EMAIL PROTECTED] wrote:

 While fixing some bugs in zope.app.container,
 I've also modified the implementation of the BTreeContainer,
 by not inheriting from the SampleContainer, and directly accessing the
 btree. This had remained as a TODO in the btree.py file, so I did it, but...

 The result is all previous persisted BTreeContainers (such as
 PrincipalFolder) are broken because the btree used to be stored in
 self._SampleContainer__data, while the new implementation stores it in
 self._BTreeContainer__data.

 So I've added a property to offer a transparent backward compatibility:

[snip]

 Do you think it is safe? Is there any better solution for this? Should I
 rather write an evolution script? Or should I revert all this back to
 inheriting from SampleContainer?

Another option would be to leave the attribute with the wrong name and
just add a note that it's for backward compatible.
-- 
Benji York
Senior Software Engineer
Zope Corporation
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )


Re: [Zope-dev] new BTreeContainer implementation, please review

2008-06-13 Thread Marius Gedminas
On Fri, Jun 13, 2008 at 09:40:06PM +0200, Christophe Combelles wrote:
 While fixing some bugs in zope.app.container,
 I've also modified the implementation of the BTreeContainer,
 by not inheriting from the SampleContainer, and directly accessing the 
 btree. This had remained as a TODO in the btree.py file, so I did it, 
 but...

 The result is all previous persisted BTreeContainers (such as 
 PrincipalFolder) are broken because the btree used to be stored in 
 self._SampleContainer__data, while the new implementation stores it in 
 self._BTreeContainer__data.

 So I've added a property to offer a transparent backward compatibility:

 def _get__data(self):
 try:
 return self._BTreeContainer__data
 except:

Please do not use bare except clauses.  Replace this with

  except AttributeError:

 return self._SampleContainer__data
 def _set__data(self, value):
 try:
 self._BTreeContainer__data = value
 except:

When could this ever fail?

 self._SampleContainer__data = value
 def _del_data(self):
 try:
 del self._BTreeContainer__data
 except:
 del self._SampleContainer__data

Do we need __del__ at all?

 __data = property(_get__data, _set__data, _del_data)



 Do you think it is safe?

Not if you want compatibility in both directions.  If you want databases
created with the new zope.app.container to be readable with the old
zope.app.container (consider, e.g. a developer using a sandbox and
switching between bleeding-edge development and old maintenance
branches), then you can't do this.

 Is there any better solution for this?

Perhaps a __setstate__ method to rename the attribute?  See the
PersistentMapping class in persistent/mapping.py for an example.

 Should I 
 rather write an evolution script?

*loathes*

 Or should I revert all this back to 
 inheriting from SampleContainer?

Or you could do like Benji York suggested and always use the
backwards-compatible name _SampleContainer__data.

(Why oh why did SampleContainer want to use a double underscored name?)

Marius Gedminas
-- 
Of course I use Microsoft. Setting up a stable unix network is no challenge ;p


signature.asc
Description: Digital signature
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )