On Jan 25, 2009, at 4:09 AM, Martijn Pieters wrote:

On Sat, Jan 17, 2009 at 00:04, Andrew Burkhalter
<andrewburkhal...@gmail.com> wrote:
PLIP 240 (Improve locking configurability) has been implemented.  You
can get the review buildout from:
https://svn.plone.org/svn/plone/review/plip240-locking-improvements/

This PLIP gets a +1 from me.

I've looked at the code changes for the 4 affected packages, ran the tests and
tested the functionality manually.

I have but one remark: the ILockable interface has been extended requiring users of the interface to test if the additional method is actually present in implementors. This test smells icky to me; better to assume the method is there (and risk breaking 3rd party implementations), or define a new interface
that extends ILockable (IRefreshableLockable?). Then register
LockingOperations for that interface instead or test for IRefreshableLockable
instead of a hasattr test.

However, I feel that removing that icky smell is not a prerequisite for
acceptance, just advice to improve what otherwise is an excellent
implementation.


I actually first implemented it exactly that way (even called it IRefreshableLockable), then wondered if the complexity was worth it. I can go either way, but would like to hear some other opinions of best practice in a case like this.

peace,


David Glick
Web Developer
ONE/Northwest

New tools and strategies for engaging people in protecting the environment

http://www.onenw.org
davidgl...@onenw.org
work: (206) 286-1235 x32
mobile: (206) 679-3833

Subscribe to ONEList, our email newsletter!
Practical advice for effective online engagement
http://www.onenw.org/full_signup





_______________________________________________
Framework-Team mailing list
Framework-Team@lists.plone.org
http://lists.plone.org/mailman/listinfo/framework-team

Reply via email to