On 21/03/15 18:58, Matthew Toseland wrote: > On 21/03/15 17:45, Ian Clarke wrote: >> Talking to a few people, I think our current approach to code review is >> problematic. >> >> For example, I've been told that some people are arguing that commits are >> too granular, and need to be combined to make code review easier. This is a >> mistake, there is nothing wrong with very granular commits, just as there >> is nothing wrong with more verbose code if it helps clarity. Pull requests >> should be used for code review, not individual commits. The number of >> individual commits should be irrelevant. >> >> It sounds like people are trying to use commits for code review, whereas >> they should be using Github pull requests. >> >> Here is a process that has worked well in my experience: >> >> - For any isolatable feature or bugfix, create a new branch just for >> that feature or bug request (perhaps put the bug id # in the name of the >> branch). *Do not combine multiple features or bugfixes into a single >> branch.* If it can be merged independently, it should have it's own >> branch. >> - Commits to this branch can be as granular as the programmer wants, >> generally the more frequent the commits the better >> - When the programmer is ready for feedback, they should create a pull >> request between this branch and the main branch - they could post the URL >> of the pull request to IRC to encourage feedback >> - It's fine for a programmer to request feedback before the feature is >> complete, but they should note this in the title of the pull request - eg. >> "DO_NOT_MERGE" >> - For code review, the most useful tab is "Files changed" - which shows >> all differences between the feature branch and the main branch (individual >> commits are irrelevant here) - comments can be added here >> - Once the feature is complete and the review feedback has been read and >> perhaps acted upon by the programmer working on the feature, it should be >> merged >> >> The person contributing the code is responsible for asking for and >> incorporating feedback, but they control the process, the process should >> not control them. So, for example, in some cases the programmer might >> decide that a code review is unnecessary. That will be their call. Better >> to trust human judgement over a rigid process. >> >> Thoughts? >> >> Ian. > Most of the above boils down to "review the diff, not the history". That > is probably sensible. It implies that the RM's can reasonably ask for some sort of guide to the changes to make diff-level review of big patches easier. > The last point is "everyone can commit anything without review". That's > the fundamental question here: Do we want to require that some > responsible person (release manager, person with push rights) reviews > and signs off on the code before it is pushed? > > There are 2 main reasons for this: > 1. Security. How useful this is is debatable. > 2. Disruptive changes to APIs.
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
