Re: [Framework-Team] PLIP #239 ready for review
On Sun, Jan 11, 2009 at 18:56, Martin Aspeli optilude+li...@gmail.com wrote: I've just finished the base implementation of PLIP #239, adapterise the ExtensibleIndexableObjectWrapper, for your review. This PLIP gets my +1. As usual, Martin delivers. Great implementation, great documentation. I did add one sentence to the README to clarify why IIndexer adapters cannot be used for workflow variables; it was a question I had while reviewing. No further comments required. :-) -- Martijn Pieters ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
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
Re: [Framework-Team] PLIP #247 ready for review (I think)
On Sat, Jan 17, 2009 at 02:37, Ethan Jucovy ethan.juc...@gmail.com wrote: I've implemented PLIP #247, Automate ZCML Loading for Plone Plugins (http://plone.org/products/plone/roadmap/247) and committed a review buildout here: https://svn.plone.org/svn/plone/review/plip247-automate-plugin-zcml for your review. I have started my review. To my dismay, however, the z3c.autoinclude package has several test failures. I understand that this package is also being used by the Grok project, and it appears to work as advertised, but test failures is not a good sign of package quality. For example, utils.txt somehow triggers an assert on line 99 in utils.py, which has a somewhat distressing comment: assert len(non_namespaced_dists) == 1 ### we really are in trouble if we get into a situation with more than one Could you comment on the state of these failures? The buildout is branched from the 3.3 review bundle buildout based on buildout.eggtractor, which automatically generates ZCML slugs for development packages and puts them in parts/instance/etc/package- includes. Since the purpose of PLIP 247 is to automate ZCML loading, those automatically generated ZCML slugs should be removed. I've hardly ever used buildout so I'm not sure how to change that behavior -- sorry about that; I'll change it if I can figure out how. Simply *not* use buildout.eggtractor; you only have 3 eggs in this buildout, add them to the develop, eggs and zcml lines and Bob's your uncle. Alternatively, just set tractor-autoload-zcml to false, which you did. -- Martijn Pieters ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #247 ready for review (I think)
On Sat, Jan 17, 2009 at 02:37, Ethan Jucovy ethan.juc...@gmail.com wrote: I've implemented PLIP #247, Automate ZCML Loading for Plone Plugins (http://plone.org/products/plone/roadmap/247) and committed a review buildout here: https://svn.plone.org/svn/plone/review/plip247-automate-plugin-zcml for your review. Here is my full verdict, as committed to the bundle readme: I've updated the buildout to not use buildout.eggtractor; you already had disabled it's zcml autoloading but didn't add Products.CMFPlone to the zcml line. Not using buildout.eggtractor at all is a better idea here, however. I've also added a foo.cfg buildout configuration to test the (very nice!) demonstration package. Just run bin/buildout -c foo.cfg to have the foo package included. After these changes, the z3c.autoinclude package appears to work as advertised. However, when running the z3c.autoinclude tests I see test failures, which worries me. I understand that the package is being used by Grok as well, and this PLIP merely covers the inclusion of the package into Plone, but I'd like to see some comments about what these test failures are about. Until I see comments that alleviate my concerns about the test failures, I'm giving this PLIP a +0. -- Martijn Pieters ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
