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.
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
