[Framework-Team] Reviewing PLIPS sux0rs | Github growing pains
witsch at plone.org
Fri Jan 20 09:12:30 UTC 2012
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…
>> 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! :)
that said, i think the only thing i'm missing there is the possibility to somehow mark or hide or fold a comment that's already been taken care of. this shouldn't be all too hard to implement, though. that's to say perhaps the friendly github folks will consider implementing it if we ask them… :)
More information about the Framework-Team