Re: [Framework-Team] Re: PLIP 237 ready for review
On Feb 5, 2009, at 10:44 PM, Hanno Schlichting wrote: Andreas Zeidler wrote: in short: i'm +1 for merging, but PLT has an error which needs to be fixed before the next release. it's not related to the changes, though. Can you add a ticket for the bug you encountered? From the review notes alone, I wasn't able to understand what the problem really is. i'll give you a demo on monday! ;) 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] Re: Final review report
On Feb 5, 2009, at 10:41 PM, Hanno Schlichting wrote: I haven't looked at the code, but why restrict this to those container interfaces alone? Isn't some IContainer or OFS-level interface all that is required? Once the Plone site root is supported, the code likely doesn't rely on any of the AT-semantics. it does. it simply stitches the already existing [iv]Cal support in AT together to become a proper feed. it's really just a very small add-on, started out with some 8 lines of code in the (working) prototype. but i guess some `IContainer` interface might work as well, assuming that we'll see mixed AT/non-AT content soon enough... 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] Re: PLIP 237 ready for review
Andreas Zeidler wrote: > On Jan 18, 2009, at 2:44 PM, Hanno Schlichting wrote: >> I'm somewhat late with creating the review buildout for PLIP 237 (Minor >> i18n upgrades) at: >> https://svn.plone.org/svn/plone/review/plip237-minor-i18n-upgrades/ >> >> The review notes and detailed explanation is found in the review >> buildout's README.txt. > > i've added notes in http://dev.plone.org/plone/changeset/24772 > > in short: i'm +1 for merging, but PLT has an error which needs to be > fixed before the next release. it's not related to the changes, though. Can you add a ticket for the bug you encountered? From the review notes alone, I wasn't able to understand what the problem really is. Thanks! Hanno ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
[Framework-Team] Re: Final review report
Andreas Zeidler wrote: > On Feb 5, 2009, at 9:48 PM, Andreas Zeidler wrote: >> This however, resulted in a 404. It turned out, that the view was only >> registered for IATFolder and IPloneSiteRoot, but not for `Large Plone >> Folder` >> a.ka. IATBTreeFolder. I took the liberty of `fixing this myself`__ ;-) >> and it >> worked as expected. > > yep, that was indeed a rather silly oversight on my behalf. thanks for > fixing it. I haven't looked at the code, but why restrict this to those container interfaces alone? Isn't some IContainer or OFS-level interface all that is required? Once the Plone site root is supported, the code likely doesn't rely on any of the AT-semantics. Hanno ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] Re: Final review report
On Feb 5, 2009, at 10:06 PM, Wichert Akkerman wrote: Interesting, that is not how I interpret a +1 at all. To me there are two possible outcomes right now: declare a PLIP ready for merging, or declare it unready for merging for a number of reasons. perhaps to clarify a bit: my +1 actually meant "ready for merging", literally. my last mail was to second that even though some PLIPs may have received +1s there might still be room for improvement (during the revision phase). but that's not a requirement. Example: http://svn.plone.org/svn/plone/review/plip246-ical-feed/REVIEW-NOTES.txt What I mean here is that even if merged as is right now I would consider it OK yet I seriously think that at least the first two points I mention in my review notes would improve things even further. But maybe others have different views on that? With that reasoning in my opion that is a negative for merging right now with those two points as criteria that need to be met before merging later. i don't read it like that. raphael clearly said "if merged as is" he "would consider it OK". of course things can be improved, but i'd interpret that as "no showstoppers found". however, if that's not true, the vote should be different indeed. So... now that the meaning of +1 is suddenly no longer sure, can I please get a list of PLIPs that are ready for merging right now? i don't think that's necessary. we all should know what a +1 means, and that's "ready for merging as is". so unless some team members speaks up or changes their vote accordingly, you may consider all PLIPs that received +2 to be mergeable. 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] Re: Final review report
On Feb 5, 2009, at 9:48 PM, Andreas Zeidler wrote: [...] otoh, all reviewers should have informed the PLIP authors via cc (or at least i hope so!?) so they know about these (minor) issues. as for 246, i'm especially +1 and will comment on your review notes in the next mail... :) heh, gotcha! without a reply with your review notes at hand (neither from tom or you, btw :)), i'm gonna paste them in from the `REVIEW- NOTES.txt` for commenting: Framework team Plone 3.3 review #1 (Tom) [...] This however, resulted in a 404. It turned out, that the view was only registered for IATFolder and IPloneSiteRoot, but not for `Large Plone Folder` a.ka. IATBTreeFolder. I took the liberty of `fixing this myself`__ ;-) and it worked as expected. yep, that was indeed a rather silly oversight on my behalf. thanks for fixing it. [...] Most of the information entered into the event appeared in iCal however, multi-day events should be marked as 'all-day' - but that's actually a limitation of ATCT's .ics implementation and not within the scope of this PLIP. i'd consider this a bug in ATCT and would suggest you create a ticket for it! ;) you may assign it to me, though. From my perspective, there are only two issues remaining: * the cache key doesn't need to include the negotiated browser language since the output, i.e. the ical data, isn't i18n-aware. (i.e. just merge the existing solution to this from `here`__ ) yep, as you've noticed (well, after pointing it out ;)) this has already been fixed in the supplemental `collective.icalfeed` package (which was created to support older version of plone, btw, including 2.5). it also contains support for collections (see below), but i didn't want to start merging things back to the PLIP in the middle of the first review phase. well, i also was too busy anyway... * the calendar view should include the Title and description of the containing folder. Currently this is not the case and iCal defaults to `Untitled` for the title and to an empty description. good one. i'll make sure to add that during the revision phase next week. Framework team Plone 3.3 review #2 (Raphael) I reviewed this under Ubuntu using Sunbird as calendar app. great! this is helpful as i've only tested it with iCal myself. In total I can confirm Tom's observations (but with 'Large Plone Folder' support added in the mean time). Here some further remarks: * I propose to add support for collections (Topics) as well like i said, that's already done in `collective.icalfeed` and will be merged back to the PLIP. * I propose to integrate this closer into the default UI. As of now, you need to manually enter the URL to get the feed. This could be done via a document action with appropriate condition (potentailly trickey and/or expensive). At least one could add the link to the events portlet. as said elsewhere before (or i least i think i have), i'll happily add such a link or action. however, i'd really appreciate a short discussion about this as i've got too little UI expertise myself. perhaps danny and/or alex have a suggestion here? or anybody else too, of course! * Since we offer vCal views in addition to iCal views for individual events it should be straight-forward to offer that as a feed as well. Would that make sense? i think it would. frankly i simply neglected it in the initial prototype (why do the boring stuff? :)) and was too tired the night i tried to wrap up the PLIP in time for the submission deadline. reviewing it i would have suggested the same myself, though. what would be a good way to test this, btw? * I wonder what it would take to enable writing back. Sunbird happily offered me to change the imported events. It even asked me for my password on the Plone site I tested with. While I didn't get any error message nothing changed on the site either. I know this is not the scope of this plip. Just want to ask those who potentially know. i'm not sure how much work it would be, but afaik there are already other packages implementing this. i found at least one while checking for potentially already existing solutions for the ical feed, but not that i'm actually trying to find it again, i can't of course... :) anyway, i think this would be a great feature, but is out of scope for this PLIP. 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/fram
Re: [Framework-Team] Re: Final review report
On 2/5/09 9:45 AM, Raphael Ritz wrote: Andreas Zeidler wrote: hi wichert, On Feb 4, 2009, at 10:11 AM, Wichert Akkerman wrote: The deadline for the PLIP review report was four days ago. From what I can see not all reviews are in, and no report has been written yet. Can I please get a proper report which covers the PLIPs that have been reviewed properly, and a date when the rest will be ready as well? steve has compiled and posted an overview last saturday. Which I had updated on Sunday http://lists.plone.org/pipermail/framework-team/2009-February/002675.html to include my late coming reviews as well. Regarding a date when the rest will be ready is pure guess work. Of course ASAP - whenever that my be. (I hope to get my last one in tonight or tomorrow). I don't know how other reviewers see this but at least I didn't consider my positive responses in all cases to mean "ready for merge" as we are entering a second round now: authors picking up on reviewer comments. Interesting, that is not how I interpret a +1 at all. To me there are two possible outcomes right now: declare a PLIP ready for merging, or declare it unready for merging for a number of reasons. There are PLIPs that are fit for merging right now. How would you score those if your +1 has another meaning? Example: http://svn.plone.org/svn/plone/review/plip246-ical-feed/REVIEW-NOTES.txt What I mean here is that even if merged as is right now I would consider it OK yet I seriously think that at least the first two points I mention in my review notes would improve things even further. But maybe others have different views on that? With that reasoning in my opion that is a negative for merging right now with those two points as criteria that need to be met before merging later. So... now that the meaning of +1 is suddenly no longer sure, can I please get a list of PLIPs that are ready for merging right now? Wichert. -- Wichert AkkermanIt is simple to make things. http://www.wiggy.net/ It is hard to make things simple. ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] Re: Final review report
On Feb 5, 2009, at 9:45 AM, Raphael Ritz wrote: I don't know how other reviewers see this but at least I didn't consider my positive responses in all cases to mean "ready for merge" as we are entering a second round now: authors picking up on reviewer comments. +1 Example: http://svn.plone.org/svn/plone/review/plip246-ical-feed/REVIEW-NOTES.txt What I mean here is that even if merged as is right now I would consider it OK yet I seriously think that at least the first two points I mention in my review notes would improve things even further. But maybe others have different views on that? no, not at all. i agree that atm +1 can still mean there are things that might be improved. i suppose we could try to update the status in that regard. otoh, all reviewers should have informed the PLIP authors via cc (or at least i hope so!?) so they know about these (minor) issues. as for 246, i'm especially +1 and will comment on your review notes in the next mail... :) 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 #247 ready for review (I think)
On Feb 5, 2009, at 9:29 PM, Andreas Zeidler wrote:
[...snip...]
yes, that works. in any case this should be documented, though.
[...snip again...]
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? :)
i forgot to mention that it'd be great if you could try to work these
out until some time next week. technically the revision deadline is
this saturday, of course, but since my review was several days late, i
think it's only fair to still grant you a full week to address these
issues. i hope the delay doesn't cause too many inconveniences and
you'll find the time it takes...
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
Re: [Framework-Team] PLIP #247 ready for review (I think)
On Feb 5, 2009, at 6:35 PM, Ethan Jucovy wrote:
Hey Andi,
hi ethan,
Thanks for the review.
thanks for the quick reply! :)
Again I'll have to be a bit brief, this time because I'm about to
get on a plane. :) But I figured I may as well give some quick
replies to keep the conversation going.
much appreciated. oh, and sorry for the late review, btw. i'm not
sure you saw my message yesterday, but unfortunately a few things kept
me from doing it earlier (lack of sleep amongst them :)).
Let me know if I should be putting these comments in SVN or on http://plone.org/products/plone/roadmap/247
instead of//as well as replying on-list, BTW.
i guess the list is fine, as interested parties can still read the
entire discussion in the archives, for example by following the link i
put on the PLIP page. you may of course also answer there or link to
your answer here. i don't think we have any strict policy in place
about this. :)
At http://dev.plone.org/plone/changeset/24863, Andreas Zeidler > wrote:
> * one issue i was wondering about is how to "target" multiple
base packages
>for the plugin case. adding an additional "target = ..." line
yields
>a "Duplicate entry point" error in `easy_install`. reading the
code
>"target = plone, grok" isn't supported either. seeing that
grok is
>already actively using the package i would imagine that
supporting
>multiple "base packages" (or projects) does make sense,
though. adding
>such support would be a definite plus, imho, and should also be
rather
>simple to do.
This is actually already a hidden feature. ;) The entry point name
("target") is purely convention; it's not used at all in the code.
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.
So
{{{
entry_points="""
[z3c.autoinclude.plugin]
morx = plone
bar = mygrok.app
"""
}}}
yes, that works. in any case this should be documented, though.
Big +1 on considering the testing complications. (Though in my
limited experience I've actually found that any of my tests that
break as a result of z3c.autoinclude's "unpredictable" ZCML
inclusion are badly isolated tests anyway.)
well, i was thinking of a debug session where some tests where failing
after LinguaPlone had been added to the buildout. i'm not entirely
sure anymore if i had already added it to the buildout (zcml-wise) or
if the module was loaded by the testrunner thereby activating some
monkey-patches.
but you're right, that might also qualify as bad test isolation, but
every little bit that helps making this easier is much appreciated
(and i know not just by me :)).
My gut feeling is that I'd rather keep logic like what to do in test
runs, or for that matter what a test run is, out of z3c.autoinclude
though.
+1, of course. that wouldn't make sense, imho. there are too many
different testrunners for one, and there will probably be others in
the future. "knowing" them all is certainly not something
`z3c.autoinclude` should attempt.
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? :)
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
Re: [Framework-Team] PLIP #247 ready for review (I think)
Hey Andi,
Thanks for the review. Again I'll have to be a bit brief, this time because
I'm about to get on a plane. :) But I figured I may as well give some quick
replies to keep the conversation going. Let me know if I should be putting
these comments in SVN or on
http://plone.org/products/plone/roadmap/247instead of//as well as
replying on-list, BTW.
At http://dev.plone.org/plone/changeset/24863, Andreas Zeidler
wrote:
> * one issue i was wondering about is how to "target" multiple base
packages
>for the plugin case. adding an additional "target = ..." line yields
>a "Duplicate entry point" error in `easy_install`. reading the code
>"target = plone, grok" isn't supported either. seeing that grok is
>already actively using the package i would imagine that supporting
>multiple "base packages" (or projects) does make sense, though. adding
>such support would be a definite plus, imho, and should also be rather
>simple to do.
This is actually already a hidden feature. ;) The entry point name
("target") is purely convention; it's not used at all in the code. So
{{{
entry_points="""
[z3c.autoinclude.plugin]
morx = plone
bar = mygrok.app
"""
}}}
ought to do what you describe. The only reason we used the "target"
convention at all is to make it read well and to keep the documentation
simpler.
> * as for `david's and my reservations`__ wrt testing i'd like to add
>that this package only automates the otherwise manually added
>loading of zcml for additional packages in your buildout. iow, it
>doesn't make a difference compared to the situation we have right
>now: if you add a package to your buildout it's zcml will be loaded
>by the ZCML layer anyway, even if you only run specific tests for
>some other, unrelated package. so shipping with `z3c.autoinclude`
>doesn't change or make this any worse.
>
>however, imho always loading essentially all zcml contained in your
>buildout is a huge problem and should be fixed at some point. otoh
>not doing so might prevent you from finding possible conflicts or
>otherwise bad interactions between your packages more quickly. so i
>suppose ideally it should be an option to the testrunner, but disabled
>by default.
>
>now if we start depending on `z3c.autoinclude` i think this will get
>harder to fix in the future. right now the additional packages are
>pulled in by your buildout, but with this change plone itself will
handle
>this. so the zcml loading `z3c.autoinclude` should at least be made
>conditional. that way you could simply disable `z3c.autoinclude` (i'm
>thinking of something like ``eggs -= z3c.autoinclude` here) and prevent
>everything being loaded. this change could then even turn out to be
>helpful wrt fixing the above mentioned issue, since `z3c.autoinclude`
>alone could be changed to not trigger zcml loading during test runs.
Big +1 on considering the testing complications. (Though in my limited
experience I've actually found that any of my tests that break as a result
of z3c.autoinclude's "unpredictable" ZCML inclusion are badly isolated tests
anyway.) My gut feeling is that I'd rather keep logic like what to do in
test runs, or for that matter what a test run is, out of z3c.autoinclude
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?
Thanks again,
Ethan
___
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 Jan 17, 2009, at 2:37 AM, Ethan Jucovy wrote: Hello, hi ethan, 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've added my review notes in http://dev.plone.org/plone/changeset/ 24863 (and r24864). for now i'm +0, but this can be fixed quite easily... :) 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
Re: [Framework-Team] Re: Final review report
Andreas Zeidler wrote: hi wichert, On Feb 4, 2009, at 10:11 AM, Wichert Akkerman wrote: The deadline for the PLIP review report was four days ago. From what I can see not all reviews are in, and no report has been written yet. Can I please get a proper report which covers the PLIPs that have been reviewed properly, and a date when the rest will be ready as well? steve has compiled and posted an overview last saturday. Which I had updated on Sunday http://lists.plone.org/pipermail/framework-team/2009-February/002675.html to include my late coming reviews as well. Regarding a date when the rest will be ready is pure guess work. Of course ASAP - whenever that my be. (I hope to get my last one in tonight or tomorrow). I don't know how other reviewers see this but at least I didn't consider my positive responses in all cases to mean "ready for merge" as we are entering a second round now: authors picking up on reviewer comments. Example: http://svn.plone.org/svn/plone/review/plip246-ical-feed/REVIEW-NOTES.txt What I mean here is that even if merged as is right now I would consider it OK yet I seriously think that at least the first two points I mention in my review notes would improve things even further. But maybe others have different views on that? That's what I consider this second round now to be about. Raphael ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
