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

David Glick (GW) davidglick at groundwire.org
Fri Jan 20 17:44:46 UTC 2012


On Jan 20, 2012, at 1:12 AM, Andi Zeidler wrote:

> hi liz, david, fwt,
>
> On Jan 19, 2012, at 8:08 , David Glick wrote:
>> On 1/18/12 3:50 PM, Elizabeth Leddy wrote:
>>> 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.
>
> how about having separate buildouts for the various plips?  back with 3.x i used to (svn) switch between plips at first as well, but even that started to be too annoying/tedious soon enough.  i found the buildouts don't really take up too much disk space, and considering the saved time plus the possibility to pick up your ttw testing from the previous review round (w/o keeping Data.fs's around) made it worthwhile.
>
> imho, now with git it would make even more sense (due to the limitations of mr.dev you mentioned, wrt (not) switching between forks etc).  in any case you wouldn't need to care about where the code lives, i.e. in a branch under 'plone' or the dev's own fork…
>

I think this would certainly have some advantages. And since it would make it easier to get things right when switching it could easily make up the time lost in getting things set up in the first place.

>>> 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?
>>
>> 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.
>
> +1 on doing inline comments.   the problem david mentioned is solved when you require the devs to set up pull requests for their changes.  all the comments made on the individual commits belonging to the request are collected on the pull request itself.  there you can also add additional comments that are not directly related to the code.  furthermore, you can easily diff the whole thing, plus you automatically get updates/fixes as part of the request.  all in all, reviewing code like that can be rather good fun! :)
>

Ah, of course that's what we should do! Good idea. Although it's a bit more complicated when the PLIP involves multiple repositories, as they often do.
David



----------
David Glick
 Web Developer
 davidglick at groundwire.org
 206.286.1235x32

Engagement technology for social and environmental change.

http://www.groundwire.org




More information about the Framework-Team mailing list