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 #247 ready for review (I think)
On Feb 7, 2009, at 12:56 AM, Ethan Jucovy wrote:
On Thu, Feb 5, 2009 at 3:37 PM, Andreas Zeidler a...@zitc.de wrote:
On Feb 5, 2009, at 9:29 PM, Andreas Zeidler wrote:
i noticed that. in fact, i was trying to figure out how the
assigned value (plone) ended up in `module_name`. i
ended up looked at the entry point's (the one you get from
`pkg_resources.iter_entry_points`) `__dict__`, assuming
that some magic in `pkg_resources` would translate the target to
be meant as the module's name. but apparently
all given value end up as `module_name` in the dict. i suppose i
should have had a closer look at entry point definitions first.
Ah, yeah, a single entry point basically models a dictionary entry:
`module_name` is (part of) the entry point's value, which is
imported as a Python object; `name` is the key.
i was missing that info initially, but figured as much after a bit...
thanks for clarifying it again, though.
Perhaps z3c.autoinclude could check if
`os.environ.has_key('Z3C_AUTOINCLUDE_DISABLED')` and the test
runner and/or a buildout option could set that environment
variable?
yep, also +1. after all, that's exactly what environment variable
are for, aren't they? :)
OK, z3c.autoinclude trunk as of r96193 checks for
'Z3C_AUTOINCLUDE_PLUGINS_DISABLED' and
'Z3C_AUTOINCLUDE_DEPENDENCIES_DISABLED' in the environment.
great. just one more tiny little suggestion: how about catching the
key error in the `enable_*` function, so people can use it
unconditionally (i.e. without consulting `dependencies_disabled` first)?
$ ./bin/zopepy
from z3c.autoinclude.api import enable_dependencies
enable_dependencies()
Traceback (most recent call last):
[...]
KeyError: 'Z3C_AUTOINCLUDE_PLUGINS_DISABLED'
n/p, none of this has been too time-consuming yet :)
:)
thanks for the update, ethan. i'm changing my vote to +1.
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] second round of PLIP reviews
Andreas Zeidler wrote: hi, i've just had a look at the updates for all the PLIPs i originally reviewed and sent some comments. i won't be available for any further discussion as well as the ultimate counting etc, though, as we're leaving for vacation tomorrow. i'd appreciate if someone from the team could take over the spokesperson responsibilities, in particular doing the counting and reporting back to wichert on saturday (or making sure it gets done). any takers or do i need to pick somebody? :) I should be available this Saturday/Sunday (I hope) so I'll take this on. Raphael 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/ ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
[Framework-Team] Re: PLIP #234 Review Revisions
Calvin Hendryx-Parker wrote: 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. FWIW, I think much of Calvin's work could've gone into a 3.2.x release as bug fixes. If he doesn't break tests, and writes a few tests for truly new code, then I think that's probably sufficient in most places. Having done something similar in the past (but not read Calvin's diff in detail), I suspect most of his changes were simply to stop people making use of portal_url() when they should've used get_navigation_root(). There may be cases when we could add a defect type test to show that the navigation root didn't work before, but now does, but let's not create too much work for what is, in many cases, more about analysing a problem and applying a few surgical fixes, than writing a ton of new code. Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book ___ 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
