From: Thomas Rast Date: Sat, 26 Oct 2013 08:14:21 GMT Subject: Re: GSoC 2014: Summary so far, discussion starter: how to improve? Message-ID: <87txg4qwiq.fsf@linux-k42r.v.cablecom.net> In-Reply-To: Junio C Hamano writes: > Thomas Rast writes: > >> Theories >> ======== >> >> * Scope creep: projects tend to get blocked on some bigger >> refactoring/restructuring task that was not in the original >> proposal. (Full disclosure: I actually proposed this theory.) > I think that is a sign that the original proposal did not look > enough at the existing code, dreaming of a pie-in-the-sky shiny > features in a green-field setting. What needs to be done within the > constraint of the existing code (including a total rewrite, if > necessary, while keeping the project's codebase maintainable is part > of the healthy develpment. Hmm, yes, but it's also the only objection that I believe I have never heard, as opposed to ignored. I'm okay if we just file this under "things to consider during project proposal review". >> * Have students review some patches > > I am not sure if this would help. > > Reviewing the patches to find style violations and off-by-one errors > is relatively easy as it can be done with knowledge on a narrow > isolated part of the system. Reviewing the design to make sure that > the change fits the way how existing subsystems work, ranging from > the internal API implementation level to consistency a changed > behaviour is presented at the UI level, however, needs understanding > of the far wider entire project than only the parts of the system > the proposed change updates. It will be even more true if the chosen > topic is a cool/shiny one. I'd choose the middle path: review for code readability. What do the functions do? Are the functions and variables named after their roles? Is there anything that I cannot understand, and therefore warrants a comment? That is much more difficult than just reviewing for style, while it can (usually) be done without too much knowledge of the outside. -- Thomas Rast tr@thomasrast.ch