Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 10, 2009, at 6:55 AM, Calvin Hendryx-Parker wrote: Hi Team, hi calvin, I just committed my revisions based on the initial review of PLIP 234. thanks! I believe I have addressed all of the concerns brought up by the reviewers. unfortunately i won't have enough time left to look at your changes (leaving for vacation tomorrow). tom will do the extra round of reviewing, though, and i guess one reviewer should be enough in this case (or else maybe another team member wants to take a second look? :)). anyway, just one more comment... [...] and found that I had already written tests for the viewlet code changes that didn't get considered in your diffs, but those tests still pass. Those tests are found here: http://dev.plone.org/plone/changeset/23315 they did (as a new file). i did notice that test (which is why i added almost in almost none of the changes are actually tested ;)), but found that one was far from enough. anyway, tom will make sure there are enough now... ;) cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2.1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 12, 2009, at 1:05 PM, Andreas Zeidler wrote: [...] i did notice that test (which is why i added almost in almost none of the changes are actually tested ;)), but found that one was far from enough. anyway, tom will make sure there are enough now... ;) that said i couldn't resist to quickly check the branches for newly added commits... afaics the only tests you actually added are the ones for the calendar portlet[*], though. imho, this is not enough. i think _all_ places affected by the changes in the PLIP need to be tested for correct behaviour for the case when the site root is _not_ the navigation root. of course the other case, i.e. having the nav-root _at_ the site root like it is the default in plone, is already well-tested and not the scope of this PLIP anyway. but i don't think many people have actually used this feature before (as it was partly broken), so we really need more thorough tests for this variant (separate nav-root) now that it has become feasible... cheers, andi [*] http://dev.plone.org/plone/changeset/24893 -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2.1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 12, 2009, at 1:19 PM, Andreas Zeidler wrote: of course the other case, i.e. having the nav-root _at_ the site root like it is the default in plone, is already well-tested and not the scope of this PLIP anyway. but i don't think many people have actually used this feature before (as it was partly broken), so we really need more thorough tests for this variant (separate nav-root) now that it has become feasible... forgot to add that i'll trust tom (and the rest of the team) to make an ultimate judgement and will go along as far as my vote is concerned... :) cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2.1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 12, 2009, at 7:19 AM, Andreas Zeidler wrote: of course the other case, i.e. having the nav-root _at_ the site root like it is the default in plone, is already well-tested and not the scope of this PLIP anyway. but i don't think many people have actually used this feature before (as it was partly broken), so we really need more thorough tests for this variant (separate nav-root) now that it has become feasible... I'd be open for suggestions for other tests. A majority of the fixes were templates and they were to use an already existing bit of code functionality that I didn't add like you said. The viewlets were modified to support this and I have the test to confirm it in the base class for the viewlets. If there are some specific tests you'd like to see I'd be happy to add them, but in the end the trickiest piece of code I updated was the calendar tool modifications and I added tests there also. FYI... we have released collective.lineage which depends on many of these fixes and I believe will expand the number of people using this functionality. There are a lot of people excited about this new product and are wanting to deploy it. We actually have 1 site in production with it already. Since this functionality was already in Plone before I got ahold of it I still see this PLIP as a large bug fix that really should be included. Cheers, Calvin -- S i x F e e t U p , I n c . http://www.sixfeetup.com Phone: +1 (317) 861-5948 x602 cal...@sixfeetup.com ANNOUNCING the first Plone Immersive Training Experience | Sept. 10-11-12, 2009 http://www.sixfeetup.com/immerse ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On 12.02.2009, at 13:19, Andreas Zeidler wrote: On Feb 12, 2009, at 1:05 PM, Andreas Zeidler wrote: [...] i did notice that test (which is why i added almost in almost none of the changes are actually tested ;)), but found that one was far from enough. anyway, tom will make sure there are enough now... ;) that said i couldn't resist to quickly check the branches for newly added commits... afaics the only tests you actually added are the ones for the calendar portlet[*], though. imho, this is not enough. i think _all_ places affected by the changes in the PLIP need to be tested for correct behaviour for the case when the site root is _not_ the navigation root. of course the other case, i.e. having the nav-root _at_ the site root like it is the default in plone, is already well-tested and not the scope of this PLIP anyway. but i don't think many people have actually used this feature before (as it was partly broken), so we really need more thorough tests for this variant (separate nav-root) now that it has become feasible... I have reviewed the changes in the documentation and in the three affected packages since december and have conducted another local TTW test. The test showed that the application of INavigationRoot worked as advertised. While I personally still feel that the switch of the root in the navigation tree once you reach a new root is kind of 'creepy' I realize that a) this is intended and b) that in actual deployment the sub-roots are usually served under different domains from the front end webserver, so the effect won't be observable to users. While I agree (with andi), that in a perfect world, this PLIP would come with more tests, I also agree (with calvin, optilude etc.) that this PLIP is by nature more a bug fix than a new feature. I don't think we can reasonably expect that all fixes to Plone must come with 100% test coverage, or else they will not be accepted. Seeing that most changes are actually in template markup and not so much in code and given the test of the base portlet and that no existing tests break I vote +1 on this. Can I guarantee that it absolutely won't break anything? No. Am I convinced that Plone is better with this PLIP than without it? Yes :-) cheers, andi [*] http://dev.plone.org/plone/changeset/24893 -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2.1 released! -- http://plone.org/products/plone/ ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 12, 2009, at 11:24 PM, Tom Lazar wrote: Can I guarantee that it absolutely won't break anything? No. Am I convinced that Plone is better with this PLIP than without it? Yes :-) :) andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2.1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
thanks, calvin! i'll take a look at it ASAP, which probably will mean saturday, though... also thanks for the changeset url, that kind of stuff is really helpful for reviewers (*hint* *hint* to other list members ;-) cheers, tom On 10.02.2009, at 06:55, Calvin Hendryx-Parker wrote: Hi Team, I just committed my revisions based on the initial review of PLIP 234. I also have included some more docs about the PLIP here: http://dev.plone.org/plone/browser/review/plip234-inavigationroot-fixes/PLIP_234_README.txt I believe I have addressed all of the concerns brought up by the reviewers. There is a summary of these changes in the PLIP_234_README.txt file. I added tests for some new changes to the calendar tool and found that I had already written tests for the viewlet code changes that didn't get considered in your diffs, but those tests still pass. Those tests are found here: http://dev.plone.org/plone/changeset/23315 Please let me know if you have any questions at all. Cheers, Calvin -- S i x F e e t U p , I n c . http://www.sixfeetup.com Phone: +1 (317) 861-5948 x602 cal...@sixfeetup.com ANNOUNCING the first Plone Immersive Training Experience | Sept. 10-11-12, 2009 http://www.sixfeetup.com/immerse ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 8, 2009, at 5:17 PM, Tom Lazar wrote: it would be great, though, if you could deliver the final version before wednesday, then i could review it on the last day of the berlinale-sprint here, after that i will be really busy with catching up on stuff. +1. i won't be able to review after wednesday as we'll be going on vacation. not being able to review also mean i cannot possibly change my vote, either, though... ;) andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2rc1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
absolutely fine by me. it would be great, though, if you could deliver the final version before wednesday, then i could review it on the last day of the berlinale-sprint here, after that i will be really busy with catching up on stuff. cheers, tom On 08.02.2009, at 07:24, Calvin Hendryx-Parker wrote: Hi All, I have addressed most of the issues in the review of my PLIP, but I have a couple more tests in addition to the ones I have added that I'd like to include before the final review is made. I'd very much appreciate it if I could have another day to include these so I can be sure that my PLIP will be included with Plone 3.3. Thanks, Calvin -- S i x F e e t U p , I n c . http://www.sixfeetup.com Phone: +1 (317) 861-5948 x602 cal...@sixfeetup.com ANNOUNCING the first Plone Immersive Training Experience | Sept. 10-11-12, 2009 http://www.sixfeetup.com/immerse ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #234 Review Revisions
On Feb 8, 2009, at 7:24 AM, Calvin Hendryx-Parker wrote: I have addressed most of the issues in the review of my PLIP, but I have a couple more tests in addition to the ones I have added that I'd like to include before the final review is made. I'd very much appreciate it if I could have another day to include these so I can be sure that my PLIP will be included with Plone 3.3. fine by me. let us know when the bundle's ready for another review. cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2rc1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
[Framework-Team] PLIP #234 Review Revisions
Hi All, I have addressed most of the issues in the review of my PLIP, but I have a couple more tests in addition to the ones I have added that I'd like to include before the final review is made. I'd very much appreciate it if I could have another day to include these so I can be sure that my PLIP will be included with Plone 3.3. Thanks, Calvin -- S i x F e e t U p , I n c . http://www.sixfeetup.com Phone: +1 (317) 861-5948 x602 cal...@sixfeetup.com ANNOUNCING the first Plone Immersive Training Experience | Sept. 10-11-12, 2009 http://www.sixfeetup.com/immerse ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
