[Framework-Team] Reviewing PLIPS sux0rs | Github growing pains

David Glick davidglick at groundwire.org
Thu Jan 19 07:08:20 UTC 2012

Some responses below...

On 1/18/12 3:50 PM, Elizabeth Leddy wrote:
> Yo FWT peeps -
> I just finished reviewing a plip, the first time not in svn and I think there are some things that to happen for me to actually review more plips and not be intensely frustrated. I suspect anyone who reviewed plips already would agree with some if not all of these. People won't do reviews if they are frustrated and/or it takes freaking forever.
> These complaints are mostly rooted in the new github setup. I know how *I* use github and I hear there are a lot of reasons why we don't use it differently for plip reviews. I am not qualified to make suggestions because my head spins with all the arguments back and forth, I was hoping to just complain and then have you guys tell me to either can it or maybe we can work out something resembling sanity.
> Forking Mad: People love forking!
> 1. When people fork, it means that I have to delete my src/Plone.CMFPlone folders and any other folder that's now elsewhere, then rerun the long arduous process of downloading that honkeriffic package. (Grr).  We need a best practice here. Would branch the coredev buildout for each PLIP solve this problem? How can we get people to stay in the plone namespace on github? Can/should we require that namespace as part of the condition of getting into core?

+1; I think we should make it a requirement that branches submitted for 
PLIP review should be within the plone organization on github. I don't 
think there's any reason people can't fork at the start if it makes them 
feel less self-conscious as they start working. But by the time a PLIP 
is submitted for review any forks should be merged into branches within 
the plone organization. In addition to removing the obstacles to 
switching between PLIPs with mr.developer, this decreases the chances 
that code will get submitted for PLIP review by people who haven't 
actually gone through the contributor agreement process yet (though with 
git, to make about code ownership, we really ought to have a script to 
check all commits after they are pushed and notify someone if there's a 
> 2. I don't know where to find code commits for review in a sane way. Looking on the file system is nutty for large packages. I just spent 15 minutes trying to find a changeset by following forks, only to see that the only line added to the CMFCore was plone.packagename in yet another fork of someone else. (Grr)

Not sure I understand this one. Can't you just do "git diff 
origin/master other_remote/some_branch_name" to see the differences 
between them? (You may need to set up both remotes first...but requiring 
the plone organization as above should help with that.) PLIP authors 
should of course be required to submit a list of what packages they 
changed, but that's usually easy enough to find by looking at the PLIP's 
adjustments to the sources in the plip .cfg.
> Documentation:
> 3. Documentation is confusing AGAIN. It is tradition to use README in gihub to provide documentation. Should we be encouraging that with PLIP updates and packages? I personally like it, but then we have yet another place to have documentation. Also, when people have forked or are hosting in their account instead of under plone, holy whoah the docs could disappear. And what about collective? Can we set on a standard for at least one place for all new documentation? (Moar Grr)

I'm going to punt on this one for now, other than to say that PLIP 
authors need to be given more guidance here. Most developers are quite 
capable of writing good documentation of their stuff, but not without 
someone telling them exactly what they need to write and where it needs 
to go.

My current thoughts about where different types of documentation go...
* A package's README *must* have at least a few paragraphs explaining 
what the package is and why it exists. It is also a great place for 
package-specific documentation (i.e. of the more detailed reference-y 
type...the pypi pages for plone.app.registry and plone.app.caching are 
great examples)
* collective docs is for helping people get oriented to the system as a 
whole and how it fits together. A good place to explain concepts and 
link to more detailed documentation for each topic and package.
* We have a real need for some good walkthrough tutorials introducing 
people to different subsystems. This is a bit of a gap for us right now. 
There's some great stuff on plone.org but it's a bit hard to find and 
we're missing tutorials for many of the newer things.
* For now documentation on upgrades should get added to the upgrade 
manual on plone.org, but we may want to think about sphinxifying this.

> Reviewing:
> 4. I personally want to comment line by line in the code and did on this last review. This is SO much easier than putting reports in the text reviews, but then the other code reviewers need to look in the same place. Can we all agree to switch to commenting on commits and lines in github at the source instead of the review.txt file?
> 5. Filing bugs. If we are in github and the package is forked, it seems like we should file the bug there. Or do we keep it in trac? I don't know what the current philosophy is. Maybe we just listen to the devs? I just want them to 1) get the bug and 2) fix it.

Doing reviews as comments on lines of code has a similar tradeoff to 
ZCML vs. grok configuration...inline comments are nice for context, but 
suck in that for the developer there's no one place to go and see a list 
of all the feedback that needs to be dealt with. Although I guess it 
sends an email so the develop can at least file those. A good code 
review has some items that don't really belong on particular lines of 
code (feedback on architecture, UX, code coverage, etc.) so I'm not sure 
what we do with those.

Re filing bugs, I think github makes the most sense in the case where 
it's a new package that's not part of core yet. I'm a bit hesitant about 
using github for filing issues on PLIPs related to packages that are 
part of core already, just because it seems like maybe we should keep 
all those in one place (the status quo being trac).

> Other:
> 6. Can we make directories in the plips to make autocomplete easier? Maybe having a directory for each plip that then has everything about that plip? My only concern is making sure the cfgs still run. The current directory layout makes my eyes bleed after so many plips.

This is just an idea I haven't thought through, but now that we have git 
and it's easier to merge, perhaps each PLIP should be a branch of 
coredev rather than a separate .cfg on coredev's master. Someone want to 

> Suggestions:
> I think at minimum, we need to have the PLIP implementors help us by *clearly* listing *urls* to
>   - All documentation, which should be at a url (should it always be in collective docs? should human docs go there and sphinx docs elsewhere? What about upgrade notes?)
>   - Which bug tracker they want us to use (that or we tell them, but I don't think that will work)
>   - A URL which directs us to all packages that have been changed.
> On our end, It would be nice to provide a guide that tells the implementors...
>   - all docs of type x go here, all docs of type y go here, all plips require docs of type X&  Y
>   - fork/branch code like this
>   - submit for review with the urls above in implementation notes (plipXXX-XXX.txt)
> This way we don't have a guessing game on both ends and additionally it get's way easier for people to add plips. I'm happy to do the legwork of documenting if I can get some consensus on where to put things and the best way to address these problems.
+ lots


David Glick
 Web Developer
 davidglick at groundwireconsulting.com

Groundwire Consulting is here.


More information about the Framework-Team mailing list